Bug 212288 – Make sure bundle identifier testing override is set in the network process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

The bundle identifier gets passed to the network process when it is initialized. If a test changes the bundle identifier for some purpose, it should make sure it initializes a new network process.

[Kate Cheney]

<rdar://problem/63539061>

[Kate Cheney]

Created attachment 400086 [details]
Patch

[Chris Dumez]

Comment on attachment 400086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400086&action=review

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:276
> +    terminateNetworkProcess();

Why cannot you update the application bundle identifier in the existing Network Process? The current _setApplicationBundleIdentifier SPI in on the view. How can you guarantee that the network process has not launched yet when you call _setApplicationBundleIdentifier ? This seems like a super fragile SPI. 

I don't think that terminating a network process is a nice way to deal with this.

[Kate Cheney]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 400086 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400086&action=review
> 
> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:276
> > +    terminateNetworkProcess();
> 
> Why cannot you update the application bundle identifier in the existing
> Network Process? The current _setApplicationBundleIdentifier SPI in on the
> view. How can you guarantee that the network process has not launched yet
> when you call _setApplicationBundleIdentifier ? This seems like a super
> fragile SPI. 
> 
> I don't think that terminating a network process is a nice way to deal with
> this.

Do you mean send IPC to the Network Process to update the bundleID there when _setApplicationBundleIdentifier is called?

[Chris Dumez]

(In reply to katherine_cheney from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 400086 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=400086&action=review
> > 
> > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:276
> > > +    terminateNetworkProcess();
> > 
> > Why cannot you update the application bundle identifier in the existing
> > Network Process? The current _setApplicationBundleIdentifier SPI in on the
> > view. How can you guarantee that the network process has not launched yet
> > when you call _setApplicationBundleIdentifier ? This seems like a super
> > fragile SPI. 
> > 
> > I don't think that terminating a network process is a nice way to deal with
> > this.
> 
> Do you mean send IPC to the Network Process to update the bundleID there
> when _setApplicationBundleIdentifier is called?

Yes.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 400086 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=400086&action=review
> > > 
> > > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:276
> > > > +    terminateNetworkProcess();
> > > 
> > > Why cannot you update the application bundle identifier in the existing
> > > Network Process? The current _setApplicationBundleIdentifier SPI in on the
> > > view. How can you guarantee that the network process has not launched yet
> > > when you call _setApplicationBundleIdentifier ? This seems like a super
> > > fragile SPI. 
> > > 
> > > I don't think that terminating a network process is a nice way to deal with
> > > this.
> > 
> > Do you mean send IPC to the Network Process to update the bundleID there
> > when _setApplicationBundleIdentifier is called?
> 
> Yes.

Seems wrong for a method on the view to behave differently whether the network process is already launched or not, even if it is SPI.

[Kate Cheney]

(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to katherine_cheney from comment #4)
> > > (In reply to Chris Dumez from comment #3)
> > > > Comment on attachment 400086 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=400086&action=review
> > > > 
> > > > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:276
> > > > > +    terminateNetworkProcess();
> > > > 
> > > > Why cannot you update the application bundle identifier in the existing
> > > > Network Process? The current _setApplicationBundleIdentifier SPI in on the
> > > > view. How can you guarantee that the network process has not launched yet
> > > > when you call _setApplicationBundleIdentifier ? This seems like a super
> > > > fragile SPI. 
> > > > 
> > > > I don't think that terminating a network process is a nice way to deal with
> > > > this.
> > > 
> > > Do you mean send IPC to the Network Process to update the bundleID there
> > > when _setApplicationBundleIdentifier is called?
> > 
> > Yes.
> 
> Seems wrong for a method on the view to behave differently whether the
> network process is already launched or not, even if it is SPI.

Agreed, I didn't think about that. I'll fix it.

[Kate Cheney]

Created attachment 400278 [details]
Patch

[Kate Cheney]

Created attachment 400282 [details]
Patch

[Kate Cheney]

Created attachment 400288 [details]
Patch

[Chris Dumez]

Comment on attachment 400288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400288&action=review

r=me with fixes.

> Source/WebKit/ChangeLog:9
> +        No new tests, will fix http/tests/in-app-browser-privacy/ tests.

The part about tests usually comes after the description, not before.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2757
> +void NetworkProcess::updateBundleIdentifierInNetworkProcess(const String&& bundleIdentifier, CompletionHandler<void()>&& completionHandler)

'String&&' or 'const String&', but not 'const String&&'. Here I would just use String&&.

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:187
> +    UpdateBundleIdentifierInNetworkProcess(String bundleIdentifier) -> () Async

Please strip 'InNetworkProcess'. This is an IPC to the network process so it is redundant.

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:188
> +    ClearBundleIdentifierInNetworkProcess() -> () Async

ditto.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1540
> +void NetworkProcessProxy::updateBundleIdentifierInNetworkProcess(const String&& bundleIdentifier, CompletionHandler<void()>&& completionHandler)

const String&, not const String&&.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2488
> +void WebsiteDataStore::updateBundleIdentifierInNetworkProcess(String bundleIdentifier, CompletionHandler<void()>&& completionHandler)

String -> const String&

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2493
> +        processPool->ensureNetworkProcess().updateBundleIdentifierInNetworkProcess(WTFMove(bundleIdentifier), [callbackAggregator = callbackAggregator.copyRef()] { });

use-after-move of bundleIdentifier if there is more than one loop iteration. Just use bundleIdentifier without moving.

[Kate Cheney]

Comment on attachment 400288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400288&action=review

Thanks! I'll make these changes before landing.

>> Source/WebKit/ChangeLog:9
>> +        No new tests, will fix http/tests/in-app-browser-privacy/ tests.
> 
> The part about tests usually comes after the description, not before.

Will fix.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2493
>> +        processPool->ensureNetworkProcess().updateBundleIdentifierInNetworkProcess(WTFMove(bundleIdentifier), [callbackAggregator = callbackAggregator.copyRef()] { });
> 
> use-after-move of bundleIdentifier if there is more than one loop iteration. Just use bundleIdentifier without moving.

Oops, good catch.

[Kate Cheney]

Created attachment 400369 [details]
Patch for landing

[EWS]

Committed r262212: <https://trac.webkit.org/changeset/262212>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400369 [details].