Bug 212288 - Make sure bundle identifier testing override is set in the network process
Summary: Make sure bundle identifier testing override is set in the network process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-22 16:34 PDT by katherine_cheney
Modified: 2020-05-27 13:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.71 KB, patch)
2020-05-22 16:39 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (14.32 KB, patch)
2020-05-26 14:53 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2020-05-26 15:49 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (14.46 KB, patch)
2020-05-26 16:45 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (14.21 KB, patch)
2020-05-27 13:09 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2020-05-22 16:34:29 PDT
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.
Comment 1 katherine_cheney 2020-05-22 16:35:01 PDT
<rdar://problem/63539061>
Comment 2 katherine_cheney 2020-05-22 16:39:57 PDT
Created attachment 400086 [details]
Patch
Comment 3 Chris Dumez 2020-05-22 16:44:41 PDT
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.
Comment 4 katherine_cheney 2020-05-22 16:49:32 PDT
(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?
Comment 5 Chris Dumez 2020-05-22 16:50:03 PDT
(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.
Comment 6 Chris Dumez 2020-05-22 16:51:21 PDT
(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.
Comment 7 katherine_cheney 2020-05-22 16:52:36 PDT
(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.
Comment 8 katherine_cheney 2020-05-26 14:53:02 PDT
Created attachment 400278 [details]
Patch
Comment 9 katherine_cheney 2020-05-26 15:49:17 PDT
Created attachment 400282 [details]
Patch
Comment 10 katherine_cheney 2020-05-26 16:45:43 PDT
Created attachment 400288 [details]
Patch
Comment 11 Chris Dumez 2020-05-27 10:33:23 PDT
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.
Comment 12 katherine_cheney 2020-05-27 11:08:58 PDT
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.
Comment 13 katherine_cheney 2020-05-27 13:09:12 PDT
Created attachment 400369 [details]
Patch for landing
Comment 14 EWS 2020-05-27 13:38:49 PDT
Committed r262212: <https://trac.webkit.org/changeset/262212>

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