Reset the application bundle identifier between test runs

[Brent Fulgham]

In Bug 205316 we added new infrastructure to allow us to switch Application Bundle Identifier for testing purposes.

Unfortunately, this is creating some "crashes" on our bots (without stack traces) because of the following code:

    if (m_hasSetApplicationBundleIdentifier) {
        // Exit if the application bundle identifier has already been set, since it can only be set once.
        exit(1);
    }

This is a problem, because WKTR streams test input, and may interleave ‘com.apple.mail’ tests with “no-bundle-id” tests, which triggers this exit.

We should clear the bundle identifier state once a test ends, so that it is in a good state for the next test run.

[Brent Fulgham]

<rdar://problem/58844002>

[Brent Fulgham]

Created attachment 388951 [details]
Patch

[Per Arne Vollan]

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?

[Maciej Stachowiak]

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.

[Brent Fulgham]

Created attachment 389019 [details]
Patch

[Brent Fulgham]

(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.

[Brent Fulgham]

(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.

[Per Arne Vollan]

(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?

[Brent Fulgham]

Created attachment 389048 [details]
Patch

[Per Arne Vollan]

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()?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 389050 [details]
Patch for landing

[Brent Fulgham]

(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.

[WebKit Commit Bot]

Comment on attachment 389050 [details]
Patch for landing

Clearing flags on attachment: 389050

Committed r255271: <https://trac.webkit.org/changeset/255271>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.