Summary: | Reset the application bundle identifier between test runs | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||
Component: | Tools / Tests | Assignee: | Brent Fulgham <bfulgham> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, ddkilzer, jiewen_tan, mjs, pvollan, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Brent Fulgham
2020-01-27 17:50:09 PST
Created attachment 388951 [details]
Patch
Comment on attachment 388951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388951&action=review Thanks for tracking this down! > Tools/WebKitTestRunner/TestController.cpp:1089 > + setApplicationBundleIdentifier(String()); I wonder if this could hit an assert since it would set the bundle identifier after it has been queried? Comment on attachment 388951 [details]
Patch
Need to fix the build errors on gtk, wee, wincairo and win, assuming they are caused by this patch. Otherwise looks good.
Created attachment 389019 [details]
Patch
(In reply to Maciej Stachowiak from comment #4) > Comment on attachment 388951 [details] > Patch > > Need to fix the build errors on gtk, wee, wincairo and win, assuming they > are caused by this patch. Otherwise looks good. Done. (In reply to Per Arne Vollan from comment #3) > Comment on attachment 388951 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388951&action=review > > Thanks for tracking this down! > > > Tools/WebKitTestRunner/TestController.cpp:1089 > > + setApplicationBundleIdentifier(String()); > > I wonder if this could hit an assert since it would set the bundle > identifier after it has been queried? I think you are right. I think I need to make a 'clear' method that will reset that value too. These tests run on device in Release build, so I didn't notice this. (In reply to Brent Fulgham from comment #7) > (In reply to Per Arne Vollan from comment #3) > > Comment on attachment 388951 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=388951&action=review > > > > Thanks for tracking this down! > > > > > Tools/WebKitTestRunner/TestController.cpp:1089 > > > + setApplicationBundleIdentifier(String()); > > > > I wonder if this could hit an assert since it would set the bundle > > identifier after it has been queried? > > I think you are right. I think I need to make a 'clear' method that will > reset that value too. These tests run on device in Release build, so I > didn't notice this. We pass the bundle identifier on to the Networking and WebContent process when they are started as well. There might be a risk of ending up with different bundle identifiers for the various processes. Would it be possible to find a way of exiting more "cleanly", so that it will not get picked up as a crash by the scripts? Is this based on return value? Created attachment 389048 [details]
Patch
Comment on attachment 389048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389048&action=review Looks good! Should we also delete the code which exits, and the associated flag? R=me. > Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:72 > + applicationBundleIdentifierOverride() = String(); emptyString()? Comment on attachment 389048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389048&action=review >> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:72 >> + applicationBundleIdentifierOverride() = String(); > > emptyString()? Good idea. Created attachment 389050 [details]
Patch for landing
(In reply to Per Arne Vollan from comment #10) > Comment on attachment 389048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389048&action=review > > Looks good! Should we also delete the code which exits, and the associated > flag? R=me. No -- I think it's good to guard against something attempting to set the bundle identifier twice, or for us forgetting to clear it. Comment on attachment 389050 [details] Patch for landing Clearing flags on attachment: 389050 Committed r255271: <https://trac.webkit.org/changeset/255271> All reviewed patches have been landed. Closing bug. |