Closed Bug 1441133 Opened 2 years ago Closed 2 years ago

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

Categories

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

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified

People

(Reporter: roxana.leitan, Assigned: bkelly)

References

Details

Crash Data

Attachments

(2 files, 2 obsolete files)

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
Group: firefox-core-security
A similar issue was just fixed by bkelly in bug 1439879. I can reproduce on Windows, but not on OSX. Ben, can you investigate?
No longer blocks: storage-v2
Group: firefox-core-security → core-security
Component: Preferences → DOM
Flags: needinfo?(bkelly)
Product: Firefox → Core
See Also: → 1439879
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Component: DOM → DOM: Service Workers
Flags: needinfo?(bkelly)
Summary: Blocking cookies and site data causes tab crash on youtube.com → changing options to block site data causes tab crash when youtube.com is already open
Hitting this assertion is not really related to bug 1439879.
See Also: 1439879
(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!
Johann, can you remove the security flag?  I don't have permissions to do that.
Flags: needinfo?(jhofmann)
Dan, can you unhide this one, please? Thank you!
Flags: needinfo?(jhofmann) → needinfo?(dveditz)
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.
Group: core-security
Flags: needinfo?(dveditz)
Priority: -- → P2
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.
Attachment #8954100 - Flags: review?(bugmail)
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.
Attachment #8954167 - Flags: review?(bugmail)
I verified locally that the P2 patch crashes without the P1 change.
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.
Attachment #8954100 - Flags: review?(bugmail) → review+
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.
Attachment #8954167 - Flags: review?(bugmail) → review+
(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.
Added comments about not needed revokeObjectURL().  Also, make workers call self.close() to avoid delaying GC.
Attachment #8954167 - Attachment is obsolete: true
Attachment #8954766 - Flags: review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7a4546e25adc
https://hg.mozilla.org/mozilla-central/rev/275855621865
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
Status: RESOLVED → REOPENED
Flags: needinfo?(bkelly)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Remove the wp variable to avoid release build warning bustage.
Attachment #8954100 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8955177 - Flags: review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/0361460b11d6
https://hg.mozilla.org/mozilla-central/rev/562fe67796af
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Verified fixed using the latest Nightly 60.0a1 (2018-03-04) on Windows 10 x64 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Depends on: 1495285
You need to log in before you can comment on or make changes to this bug.