1441133 - changing options to block site data causes tab crash when youtube.com is already open

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[roxleitan]

Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID:20180226100105

[Affected versions]:
Nightly 60.0a1

[Affected platforms]:
Ubuntu 16.04 X64, Windows 10 x64

[Prerequisites]:
"Accept cookies and site data from websites" option is selected

[Steps to reproduce]:
1.Launch Nightly 60.0a1 with a new profile
2.Navigate to youtube.com and play a video
3.Go to about:preferences#privacy and select "Block cookies and site data" option
4.Go to the tab opened in step 2
5.Click "next video" button

[Expected result]:
The website should be displayed without issues

[Actual result]:
Tab crash: 
https://crash-stats.mozilla.com/report/index/f4a8ad57-00d7-45be-a1e9-be7af0180226

Comment 1

[Johann Hofmann [:johannh]]

A similar issue was just fixed by bkelly in bug 1439879. I can reproduce on Windows, but not on OSX. Ben, can you investigate?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I can reproduce.  Note, however, I don't see anything that makes this a security issue so far.

The issue here is the preference is being changed after a site has already started accessing storage.  When the service worker is inherited to an about:blank window (as we must per spec), then its tripping the assertion.

I'll look at fixing it, but I don't think its high criticality so far.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Hitting this assertion is not really related to bug 1439879.

Comment 4

[Johann Hofmann [:johannh]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> I can reproduce.  Note, however, I don't see anything that makes this a
> security issue so far.

Yeah, sorry, I forgot to clarify that I made this a security issue because bug 1439879 is related and I wanted to await your judgement on the criticality.

Since bug 1439879 is fixed we can open this one up, I guess.

Thanks for looking into it!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Johann, can you remove the security flag?  I don't have permissions to do that.

Comment 6

[Johann Hofmann [:johannh]]

Dan, can you unhide this one, please? Thank you!

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Don't assert storage permission on windows that inherit the service worker. r=asuth

This fixes a bunch of spurious assertions with about:blank and blob URL windows/workers that trigger because of service worker inheritance.

I believe this should also fix the youtube issue, although is challenging for me to trigger the YT thing reliably in a debug build.  The media player slows everything down in debug to the point where sometimes I can't trigger it.

I'll add a P2 patch with a test.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954100 [details] [diff] [review]
P1 Don't assert storage permission on windows that inherit the service worker. r=asuth

Andrew, this patch loosens our assertions around controlling a client that does not have access to storage.  In general we don't want a service worker to control a client without storage access, but there are some corner cases to consider.

In this case we are interested in the case where:

1. Storage is allowed
2. Client is opened and controlled
3. Storage is disallowed
4. Client from (2) creates another client that inherits the controller

In these cases we should allow the controller inheritance as its required by the spec.  The storage block will be applied on the next top level window that is opened.

The main two inheritance cases we are checking for here are "about:blank" iframes and blob URL windows/workers.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954167 [details] [diff] [review]
P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

This adds test cases for inheriting the controller after storage is denied for about:blank iframe, blob URL iframe, and blob URL worker.

There is a single TV failure in automation.  I will investigate, but I don't think its likely to be a big change from the current patch.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I verified locally that the P2 patch crashes without the P1 change.

Comment 12

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

Comment on attachment 8954100 [details] [diff] [review]
P1 Don't assert storage permission on windows that inherit the service worker. r=asuth

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

Restating: Relax assertions because "about:blank"/Blob URL documents and Blob URL workers are possible and defined to inherit the controller, and the should-be-controlled decision is latched and not subject to subsequent permission changes.  And although convention is to use nsIURI's schemeIs check or similar scheme check for "blob":
- These are all just assertions.
- ClientInfo does not have the URL in parsed form, just as a string.  But it came from a fully parsed URL and scheme parsing is straightforward (https://url.spec.whatwg.org/#url-parsing), so as long as we're checking for the scheme with the colon, it's fine.

Comment 13

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

Comment on attachment 8954167 [details] [diff] [review]
P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

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

Very nice, thorough test additions!

::: dom/serviceworkers/test/browser_storage_permission.js
@@ +155,5 @@
> +
> +  let controller2 = await ContentTask.spawn(browser, null, async function() {
> +    let b = new content.Blob(["<!DOCTYPE html><html></html>"], { type: "text/html" });
> +    let f = content.document.createElement("iframe");
> +    f.src = content.URL.createObjectURL(b);

hygiene nit: Because of the foot-gun that is createObjectURL, suggest adding a comment like "Okay to not revokeObjectURL and let URL live until tab is removed." before the invocations for the benefit of those who might cargo-cult for other tests or the web at large.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #10)
> There is a single TV failure in automation.  I will investigate, but I don't
> think its likely to be a big change from the current patch.

I fixed the TV failure by explicitly closing the workers.  I think they were delaying the GC of the window.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

Added comments about not needed revokeObjectURL().  Also, make workers call self.close() to avoid delaying GC.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4546e25adc
P1 Don't assert storage permission on windows that inherit the service worker. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/275855621865
P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7a4546e25adc
https://hg.mozilla.org/mozilla-central/rev/275855621865

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out 2 changesets (bug 1441133) for build bustage in beta simulations at dom/clients/manager/ClientSource.cpp:402: unused variable 'wp':

https://hg.mozilla.org/mozilla-central/rev/0f8f71b0b9d84e7732c07f841e395de516b31b66

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

central-as-beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5df390036cea740eabc0938b6d2102908af2977&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=165071971
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=165071953&repo=try

Uplift from central to beta is today, that's why it had to be backed out.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Don't assert storage permission on windows that inherit the service worker. r=asuth

Remove the wp variable to avoid release build warning bustage.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0361460b11d6
P1 Don't assert storage permission on windows that inherit the service worker. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/562fe67796af
P2 Verify inherited frames do not trigger service worker storage assertions. r=asuth

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0361460b11d6
https://hg.mozilla.org/mozilla-central/rev/562fe67796af

Comment 23

[roxleitan]

Verified fixed using the latest Nightly 60.0a1 (2018-03-04) on Windows 10 x64 and Ubuntu 16.04 x64.