1047105 - asmjscache: should not store cache entries when private browsing is enabled (Tor 19417)

Status: RESOLVED FIXED

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

Description

[Luke Wagner [:luke]]

In addition to not leaving a trail in the storage dir of what sites with asm.js you've visited, this would also be useful for testing purposes.

I see that IDB goes from nsIDOMWindow to nsIWebNavigation to nsILoadContext to test UsePrivateBrowsing().  Unfortunately, the asm.js cache interface is given a principal, not a window.  Can one derive the private browsing state from a principal?  If not, we could change the asm.js cache interface to take other stuff.  For workers, we have, well, all the stuff a worker has, and from a main-thread window we have a global object.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Can one derive the private browsing state from a principal?

No.  You can load the same web page in both a private browsing window and a normal one, and they will have the same principal.

Comment 2

[Luke Wagner [:luke]]

But I assume there is a straightforward path from the global object and an nsPIDOMWindow to private browsing state?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, definitely.

Comment 4

[Luke Wagner [:luke]]

Do the two have a common suffix that can be passed in through the caching interface or do you think it'd be simpler for the two callers (worker, non-worker) to handle this check themselves before calling the caching logic?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Do the two have a common suffix 

Which two?  Window and worker?

The logic needs to be very different for window and worker, so probably better to handle it outside the caching logic itself and have them pass in the private browsing state.

Comment 6

[Georg Koppen]

It is actually amazing that this whole feature landed without taking Private Browsing Mode into account and without anybody committed to working on it at least afterwards. What's more this cache does not even get deleted if one is checking "Cache" while getting rid of the whole browsing history.

That said we probably like to work on this to get it fixed.

Luke, where are we now given that almost two years have passed? Could we still follow the approach as outlined above?

Meanwhile, is there a way to disable asmjscache, like a preference we could flip or is there some way for doing that via an extension?

Is there a way to delete this cache, using e.g. an extension?

Comment 7

[Jan Varga [:janv]]

(In reply to Georg Koppen from comment #6)
> Is there a way to delete this cache, using e.g. an extension?

We now have a saner way how to "upgrade" <profile>/storage.
There's a major and minor version for <profile>/storage. Major for backward incompatible changes and minor for backward compatible. So we can bump the minor version and delete asmjs subdirs without breaking users which use the same profile with Nigthly/Aurora/Beta.

Comment 8

[Luke Wagner [:luke]]

dom/asmjscache should be removed entirely before long; wasm will have explicit caching via IDB so will work however IDB works.  I'd be happy to have any help finding out whether we are in private browsing mode so we can simply disable the asmjscache, though.

Comment 9

[Tanvi Vyas[:tanvi]]

So soon, we will be able to tell if you are in private browsing mode from the Principal's OriginAttributes.  Ehsan and James are working on moving the private browsing mode flag to Origin Attributes.

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Luke Wagner [:luke] from comment #8)
> dom/asmjscache should be removed entirely before long; wasm will have
> explicit caching via IDB so will work however IDB works.  I'd be happy to
> have any help finding out whether we are in private browsing mode so we can
> simply disable the asmjscache, though.

Do you have a timeline on this?

Comment 11

[Luke Wagner [:luke]]

When wasm ships with explicit caching (end-of-year or early 2017) would be a good time, but if there was a more immediate need and no alternative, we could remove it earlier (it's just an optimization).

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I just ran into this today while working on bug 781982, was shocked when asmjs cached even though I thought it'd be off. I'm having to distribute private browsing information for EnsureOriginIsInitialized anyways, so it should be pretty easy to make this fail with that info now.

Is it worth it for me to patch and land this before wasm lands?

Comment 13

[Luke Wagner [:luke]]

Yes, as you can see at the root of the bug, I'd be quite happy to disable in private browsing mode; it's just a challenge of getting from A->B given the limited starting point of a principal.

Comment 14

[Boris Zbarsky [:bzbarsky]]

At this point the private browsing state is stored in (Gecko) principals, so there isn't really a challenge, other than having a runtime callback to ask whether the given principal is in private browsing.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, there's multiple ways to get to the info now, so it shouldn't be too bad. I'll try to get this done early next week as part of the IDB work.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Disable AsmJSCache in Private Browsing Mode

Basic patch that seems to work with hand testing. Still trying to figure out how to mochitest this.

Comment 17

[Jan Varga [:janv]]

Comment on attachment 8786971 [details] [diff] [review]
Patch 1 (v1) - Disable AsmJSCache in Private Browsing Mode

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

::: dom/asmjscache/AsmJSCache.cpp
@@ +1549,5 @@
>      return JS::AsmJSCache_SynchronousScript;
>    }
>  
> +  uint32_t pbId;
> +  aPrincipal->GetPrivateBrowsingId(&pbId);

You need to check the result here.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Disable AsmJSCache in Private Browsing Mode

Added nsresult checking.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Disable AsmJSCache in Private Browsing Mode

Added some comments along with validity check.

Comment 20

[Jan Varga [:janv]]

Comment on attachment 8787467 [details] [diff] [review]
Patch 1 (v3) - Disable AsmJSCache in Private Browsing Mode

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

Looks good, thanks!

::: dom/asmjscache/AsmJSCache.cpp
@@ +1553,5 @@
> +  // Since AsmJSCache requires disk access at the moment, caching should be
> +  // disabled in private browsing situations. Failing here will cause later
> +  // read/write requests to also fail.
> +  uint32_t pbId;
> +  if (NS_FAILED(aPrincipal->GetPrivateBrowsingId(&pbId))) {

Nit: if (NS_WARN_IF(NS_FAILED(...

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07979189c602
Disable AsmJS caching while in Private Browsing Mode; r=janv

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07979189c602

Comment 23

[Luke Wagner [:luke]]

Thanks Kyle!