1173467 - Cache API should at least reject in private browsing mode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While bug 1117808 is intended to solve private browsing for storage in the correct long term way, we need to at least block Cache usage in the short term.  For now, lets reject.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Modify CacheStorage to reject with SecurityErr instead of throwing on creation. r=ehsan

Modify how CacheStorage creation triggers SecurityErr.  Instead of throwing on the caches attribute access, instead create a CacheStorage that simply rejects all method calls.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan

Some refactoring in nsGlobalWindow to make it easier to determine if we're in private browsing.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a WorkerPrivate private browsing flag. r=ehsan

Add flag on WorkerPrivate so we can determine off-main-thread if the worker was created in private browsing mode.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Pass private browsing flag into CacheStorage factory methods. r=ehsan

Require passing private browsing flag to CacheStorage::CreateOnMainThread().  Read the WOrkerPrivate flag in the worker case.  For the constructor inspect the global's document for a load context to determine private browsing.

For the ServiceWorkerManager and worker ScriptLoader cases we simply prevent ServiceWorkers from loading in private browsing mode.  Then we assume all later ServiceWorkerManager usage of cache is not private browsing.  This is necessary because the ServiceWorkerRegistrationInfo and friends have no idea about private browsing.

Otherwise its fairly straightforward to get the flag passed in correctly.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I will add a test tomorrow.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add a test to validate Cache in private browsing window. r=ehsan

A test to validate a private window.

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1.5 Disable wpt sandboxed-iframes.https.html test until it can be updated. r=jgraham

So the current sandboxed-iframes.https.html wpt test expects this to throw:

  self.caches

But the spec does not say this.  In fact, it now says that CacheStorage methods should do this:

  "If the result of running is settings object a secure context with the incumbent settings object is Not Secure, return a new promise rejected with a "SecurityError" exception."

Arguably a sandboxed iframe qualifies as failing this check.  So we should reject the promise instead of throwing from the global getter.

James, I would like to disable the test for now until I can get it fixed upstream in WPT.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

New try:

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

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8626398 [details] [diff] [review]
P1.5 Disable wpt sandboxed-iframes.https.html test until it can be updated. r=jgraham

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

Can you fix up the commit message to say that you're marking as expected fail rather than disabling?

Comment 10

[(no longer active)]

Comment on attachment 8626036 [details] [diff] [review]
P1 Modify CacheStorage to reject with SecurityErr instead of throwing on creation. r=ehsan

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

::: dom/cache/CacheStorage.cpp
@@ +174,5 @@
>  
> +CacheStorage::CacheStorage(nsresult aFailureResult)
> +  : mNamespace(INVALID_NAMESPACE)
> +  , mActor(nullptr)
> +  , mStatus(aFailureResult)

Please initialize the rest of the members as well.

Comment 11

[(no longer active)]

Comment on attachment 8626037 [details] [diff] [review]
P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan

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

This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing helper which takes a document.  It would be much nicer if you added an overload that takes a window, but if you don't want to do that in this bug, it can wait for a follow-up.

Comment 12

[(no longer active)]

Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

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

It seems like right now, if there is a private and a non-private window from the same origin and they both try to create a shared worker, they will get the same shared worker, which is broken.  I filed bug 1177621 about that and baku promised to fix!

But about this patch, this flag will be wrong depending on who gets to create a shared worker first.  I personally think since we're going to fix bug 1177621 soon, that is not a huge issue, but please ask a worker peer about this.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

Kyle, can you look at this one for me tomorrow?

Basically I need to know if a particular worker is for a private browsing session or not.  I set a flag based on the nsILoadGroup context used to create the worker.

Ehsan correctly notes that this is a bit broken for SharedWorker since a private and non-private window could attach to the same worker.

Ehsan wrote bug 1177621 to get the SharedWorker sharing to be fixed.  Since that will be worked separately, are you ok with this flag landing in the meantime?

(Note, we do not plan on supporting ServiceWorker interception for private browsing.)

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #10)
> Comment on attachment 8626036 [details] [diff] [review]
> P1 Modify CacheStorage to reject with SecurityErr instead of throwing on
> creation. r=ehsan
> 
> Review of attachment 8626036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/CacheStorage.cpp
> @@ +174,5 @@
> >  
> > +CacheStorage::CacheStorage(nsresult aFailureResult)
> > +  : mNamespace(INVALID_NAMESPACE)
> > +  , mActor(nullptr)
> > +  , mStatus(aFailureResult)
> 
> Please initialize the rest of the members as well.

I believe all of the other members have default values.  Their types are nsCOMPtr, UniquePtr, nsRefPtr, and nsTArray.

Correct if I'm wrong, but typically we don't require explicit initialization of these types, right?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #11)
> Comment on attachment 8626037 [details] [diff] [review]
> P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan
> 
> Review of attachment 8626037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing
> helper which takes a document.  It would be much nicer if you added an
> overload that takes a window, but if you don't want to do that in this bug,
> it can wait for a follow-up.

Well, I was trying to just de-dupe code that was in this file.  I'd prefer to avoid more wide-ranging refactoring right now if I can.

Comment 16

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #14)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #10)
> > Comment on attachment 8626036 [details] [diff] [review]
> > P1 Modify CacheStorage to reject with SecurityErr instead of throwing on
> > creation. r=ehsan
> > 
> > Review of attachment 8626036 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/cache/CacheStorage.cpp
> > @@ +174,5 @@
> > >  
> > > +CacheStorage::CacheStorage(nsresult aFailureResult)
> > > +  : mNamespace(INVALID_NAMESPACE)
> > > +  , mActor(nullptr)
> > > +  , mStatus(aFailureResult)
> > 
> > Please initialize the rest of the members as well.
> 
> I believe all of the other members have default values.  Their types are
> nsCOMPtr, UniquePtr, nsRefPtr, and nsTArray.
> 
> Correct if I'm wrong, but typically we don't require explicit initialization
> of these types, right?

No you're right, my mistake!

(In reply to Ben Kelly [:bkelly] from comment #15)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #11)
> > Comment on attachment 8626037 [details] [diff] [review]
> > P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan
> > 
> > Review of attachment 8626037 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing
> > helper which takes a document.  It would be much nicer if you added an
> > overload that takes a window, but if you don't want to do that in this bug,
> > it can wait for a follow-up.
> 
> Well, I was trying to just de-dupe code that was in this file.  I'd prefer
> to avoid more wide-ranging refactoring right now if I can.

OK, that's fine.

Comment 17

[(no longer active)]

Comment on attachment 8626350 [details] [diff] [review]
P5 Add a test to validate Cache in private browsing window. r=ehsan

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

::: dom/cache/test/mochitest/browser_cache_pb_window.js
@@ +61,5 @@
> +}
> +
> +function test() {
> +  waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set': [['browser.privatebrowing.autostart', true],

You don't need to set this pref.  This turns on permanent private browsing mode which requires a restart to take effect, and doesn't help with this test anyway.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

Obsolete this patch in favor of Andrea's solution.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a20799ebf48c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9312c5cb756e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e8bba17067
https://hg.mozilla.org/integration/mozilla-inbound/rev/af075443ab21
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cefdf8d020

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

> ::: dom/cache/test/mochitest/browser_cache_pb_window.js
> > +function test() {
> > +  waitForExplicitFinish();
> > +  SpecialPowers.pushPrefEnv({'set': [['browser.privatebrowing.autostart', true],
> 
> You don't need to set this pref.  This turns on permanent private browsing
> mode which requires a restart to take effect, and doesn't help with this
> test anyway.

I'm sorry, but I missed removing this.  I'll do it in a follow-up.

Comment 21

[Cameron McCormack (:heycam)]

This had to be backed out for:

Assertion failure: NS_IsMainThread(), at /builds/slave/m-in-lx-d-00000000000000000000/build/src/dom/bindings/BindingUtils.cpp:831
TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_app_protocol.html | application terminated with exit code 11
PROCESS-CRASH | dom/workers/test/serviceworkers/test_app_protocol.html | application crashed [@ mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, xpcObjectHelper&, nsID const*, bool)]

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a691e3ac2b

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Looking at this:

  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=30cefdf8d020

I think its pretty clear this failure was not caused by this bug.  The stack suggests its URLSearchParams which was bug 1177916 (and was also backed out).

I intend to reland this later tonight.

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1524034982b8

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aac52b9be30
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7830ef8e7c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9ba1e0e23e
https://hg.mozilla.org/integration/mozilla-inbound/rev/515147dbf455
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac30d7919319

Comment 26

[Nigel Babu [:nigelb]]

I think this landing has made https://bugzilla.mozilla.org/show_bug.cgi?id=1174977 more intermittent. I'm not confident enough to back it out, but definitely worth a look before it gets merged around.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2aac52b9be30
https://hg.mozilla.org/mozilla-central/rev/a7830ef8e7c0
https://hg.mozilla.org/mozilla-central/rev/3d9ba1e0e23e
https://hg.mozilla.org/mozilla-central/rev/515147dbf455
https://hg.mozilla.org/mozilla-central/rev/ac30d7919319

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(In reply to [e-mail] [:nigelb] from comment #26)
> I think this landing has made
> https://bugzilla.mozilla.org/show_bug.cgi?id=1174977 more intermittent. I'm
> not confident enough to back it out, but definitely worth a look before it
> gets merged around.

The "cache" in bug 1174977 is unrelated to the Cache API here.  The dom/cache and dom/workers/test/serviceworkers tests don't run in the same chunk as the test in bug 1174977 either.  I don't see how these two could be connected.  Also, bug 1174977 has been extremely frequent and intermittent for weeks now.

Please talk to me before backing out over this issue.

Comment 29

[Nigel Babu [:nigelb]]

I figured it was actually just intermittent rather than anything to do with your landing. Eventually confirmed with Tomcat that it was definitely not your fault.

Thanks for the detailed explanation :)