1426253 - Expose GetClientInfo() and GetController() on nsIDocument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

To make it easier to replace nsIDocument in service worker code I want to add a few helper methods.  Specifically I want to add:

  Maybe<ClientInfo> GetClientInfo() const;

And:

  Maybe<ServiceWorkerDescriptor> GetController() const;

These will just forward to the document's inner window, if there is one.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose nsIDocument GetClientInfo() and GetController(). r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose nsIDocument GetClientInfo(), GetClientState(), and GetController(). r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use nsIDocument::GetClientInfo() where possible. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Use the window/document GetController() method. r=asuth

I should probably add an assertion in ClientSource::SetController() that the principal does not have the private browsing origin attribute flag.  I will figure that out tomorrow since it requires extracting it from the PrincipalInfo OMT.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d941fb872f640fdd6945bfd6996ea5702922d375

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Assert that ClientSource::SetController() is never called on a client in private browsing mode. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=327f076a926088eedc82bb0d336173b20f0ba749

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937856 [details] [diff] [review]
P1 Expose nsIDocument GetClientInfo(), GetClientState(), and GetController(). r=asuth

Andrew, this patch pipes the GetClientInfo(), GetClientState(), and GetController() methods from the inner window through to the document.  This is a convenience to save a bunch of GetInnerWindow() and nullptr checks everywhere.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937880 [details] [diff] [review]
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth

There are a few call sites already that can use the GetClientInfo() and GetClientState() helpers.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937881 [details] [diff] [review]
P3 Use the window/document GetController() method. r=asuth

While looking for call sites where we can use the GetController() helper I realized we can actually remove more code.  There is a ServiceWorkerManager::IsControlled() method that we can completely remove.  Also an nsContentUtils method.

Note, these methods had nsContentUtils::IsInPrivateBrowsing() checks that we don't do any more.  I believe these are from before the private browsing id being added to the principal origin attributes.  Now that the principal is keyed on the private browsing id, I don't think we should ever match a private browsing window with a service worker registration.

I will add an assertion for this private browsing invariant in the next patch.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937898 [details] [diff] [review]
P4 Assert that ClientSource::SetController() is never called on a client in private browsing mode. r=asuth

This patch adds a ClientInfo::IsPrivateBrowsing() helper method that looks at the client's PrincipalInfo.  We then use this to assert that ClientSource::SetController() is never called in private browsing mode.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937880 [details] [diff] [review]
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth

Review of attachment 8937880 [details] [diff] [review]:
-----------------------------------------------------------------

Delightful fallout from part 1!

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937881 [details] [diff] [review]
P3 Use the window/document GetController() method. r=asuth

Review of attachment 8937881 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed that OriginAttributes and SWM's partitioning by origin covers the private browsing case.

Also, nice additional cleanup!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fb331fc914
P1 Expose nsIDocument GetClientInfo(), GetClientState(), and GetController(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b676a0c9383
P2 Use nsIDocument::GetClientInfo() where possible. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c725dad7a9
P3 Use the window/document GetController() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/661c1ca56b09
P4 Assert that ClientSource::SetController() is never called on a client in private browsing mode. r=asuth

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c7fb331fc914
https://hg.mozilla.org/mozilla-central/rev/5b676a0c9383
https://hg.mozilla.org/mozilla-central/rev/83c725dad7a9
https://hg.mozilla.org/mozilla-central/rev/661c1ca56b09