Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Revert "Sped up the objc standard message codec (#25998)" #26117

Merged
merged 1 commit into from
May 13, 2021

Conversation

cbracken
Copy link
Member

This appears to have triggered reproducible failures in channels_integration_test_ios:

[   +4 ms] 00:01 [32m+0[0m: channel suite step through[0m
[+3744 ms] Unsupported value: Sun Mar 11 07:16:42 2018 of type __NSTaggedDate
[        ] *** Assertion failure in void WriteValue(CFMutableDataRef, id)(), FlutterStandardCodec.mm:340
[   +2 ms] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Unsupported value for standard codec.'
[        ] *** First throw call stack:
[        ] (0x19d5bd9d8 0x1b1940b54 0x19d4cc50c 0x19e815238 0x1050031ec 0x104823f80 0x105003aac 0x1050009bc 0x104824e9c 0x105000b4c 0x104d0cc98 0x10501b398 0x104fb3c94 0x104fb72c4 0x19d53e3e0 0x19d53dfe4 0x19d53d4c4 0x19d537850 0x19d536ba0 0x1b429c598 0x19fe282f4 0x19fe2d874 0x1048257fc 0x19d215568)
[        ] libc++abi.dylib: terminating with uncaught exception of type NSException
[  +65 ms] Process 541 stopped
[        ] * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
[        ]     frame #0: 0x00000001c93e584c libsystem_kernel.dylib`__pthread_kill + 8
[        ] libsystem_kernel.dylib`__pthread_kill:
[        ] -> 0x1c93e584c <+8>:  b.lo   0x1c93e5868               ; <+36>
[        ]    0x1c93e5850 <+12>: stp    x29, x30, [sp, #-0x10]!
[        ]    0x1c93e5854 <+16>: mov    x29, sp
[        ]    0x1c93e5858 <+20>: bl     0x1c93c2f5c               ; cerror_nocancel
[        ] Target 0: (Runner) stopped.

Example builds:

Example Log:

This reverts commit 99021da.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

This appears to have triggered reproducible failures in channels_integration_test_ios:

    [   +4 ms] 00:01 [32m+0[0m: channel suite step through[0m
    [+3744 ms] Unsupported value: Sun Mar 11 07:16:42 2018 of type __NSTaggedDate
    [        ] *** Assertion failure in void WriteValue(CFMutableDataRef, id)(), FlutterStandardCodec.mm:340
    [   +2 ms] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Unsupported value for standard codec.'
    [        ] *** First throw call stack:
    [        ] (0x19d5bd9d8 0x1b1940b54 0x19d4cc50c 0x19e815238 0x1050031ec 0x104823f80 0x105003aac 0x1050009bc 0x104824e9c 0x105000b4c 0x104d0cc98 0x10501b398 0x104fb3c94 0x104fb72c4 0x19d53e3e0 0x19d53dfe4 0x19d53d4c4 0x19d537850 0x19d536ba0 0x1b429c598 0x19fe282f4 0x19fe2d874 0x1048257fc 0x19d215568)
    [        ] libc++abi.dylib: terminating with uncaught exception of type NSException
    [  +65 ms] Process 541 stopped
    [        ] * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    [        ]     frame #0: 0x00000001c93e584c libsystem_kernel.dylib`__pthread_kill + 8
    [        ] libsystem_kernel.dylib`__pthread_kill:
    [        ] -> 0x1c93e584c <+8>:  b.lo   0x1c93e5868               ; <+36>
    [        ]    0x1c93e5850 <+12>: stp    x29, x30, [sp, #-0x10]!
    [        ]    0x1c93e5854 <+16>: mov    x29, sp
    [        ]    0x1c93e5858 <+20>: bl     0x1c93c2f5c               ; cerror_nocancel
    [        ] Target 0: (Runner) stopped.

Example builds:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/828/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/829/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/830/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/831/overview

Example Log:
* https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8847377106855056784/+/u/run_channels_integration_test_ios/stdout

This reverts commit 99021da.
@cbracken cbracken requested a review from gaaclarke May 13, 2021 04:49
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM!

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 13, 2021
@cbracken cbracken merged commit 08d9bc0 into flutter:master May 13, 2021
@cbracken cbracken deleted the revert-channels-change branch May 13, 2021 05:43
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2021
cbracken pushed a commit to flutter/flutter that referenced this pull request May 13, 2021
* 67b046e Update documentation for embedding SplashScreen (flutter/engine#25943)

* a0a7203 Set exitcode to 0 on successful uwptool launch (flutter/engine#26105)

* ecb367f Roll Skia from 0270bf5d10be to 4e9d5e2bdf04 (5 revisions) (flutter/engine#26102)

* 85bcce7 Roll Dart SDK from 934cc986926d to 171876a4e6cf (2 revisions) (flutter/engine#26101)

* b99a362 Roll Fuchsia Mac SDK from GNyjTge9c... to q1qWG9XiN... (flutter/engine#26107)

* f7c3e7e Added exception if you try to reply with a non-direct ByteBuffer. (flutter/engine#25952)

* 96ec5a5 Roll Skia from 4e9d5e2bdf04 to 84f70136abfb (4 revisions) (flutter/engine#26108)

* 2c3c1df Use a comma-separated args string for UWP (flutter/engine#26110)

* 25c06a4 Roll Skia from 84f70136abfb to 66441d4ea0fa (2 revisions) (flutter/engine#26111)

* d21a04b Roll Skia from 66441d4ea0fa to 537b7508343d (1 revision) (flutter/engine#26113)

* aeb98f4 Roll Fuchsia Linux SDK from MoY7UVVro... to WYD7atCH7... (flutter/engine#26116)

* 08d9bc0 Revert "Sped up the objc standard message codec (#25998)" (flutter/engine#26117)
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@gaaclarke
Copy link
Member

Weird, __NSTaggedDate appears to be an internal class for representing dictionaries. The if ([value isKindOfClass:kNSDictionaryClass]) should have caught it. It seems like maybe the Class memory is being wiped out from the static variable. Since it wasn't an NSObject it wasn't apparent it had any component that was on the heap. Unfortunately that was were most of the savings came from, I'll see if I can somehow keep the Class in memory. Thanks for the revert.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes platform-ios platform-macos waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants