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

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: luke, Assigned: qdot)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tor][OA])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
> 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.
(Reporter)

Comment 2

3 years ago
But I assume there is a straightforward path from the global object and an nsPIDOMWindow to private browsing state?
Yes, definitely.
(Reporter)

Comment 4

3 years ago
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?
> 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

10 months ago
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?
Flags: needinfo?(luke)
Whiteboard: [tor]

Comment 7

10 months ago
(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.
(Reporter)

Comment 8

10 months ago
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.
Flags: needinfo?(luke)

Updated

9 months ago
Priority: -- → P3
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.
Whiteboard: [tor] → [tor][OA]
(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?
(Reporter)

Comment 11

9 months ago
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).
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?
Flags: needinfo?(luke)
(Reporter)

Comment 13

9 months ago
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.
Flags: needinfo?(luke)
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.
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.
Assignee: nobody → kyle
Summary: asmjscache: should not store cache entries when private browsing is enabled → asmjscache: should not store cache entries when private browsing is enabled (Tor 19417)
Created attachment 8786971 [details] [diff] [review]
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.
Attachment #8786971 - Flags: review?(jvarga)

Comment 17

8 months ago
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.
Attachment #8786971 - Flags: review?(jvarga)
Created attachment 8787430 [details] [diff] [review]
Patch 1 (v2) - Disable AsmJSCache in Private Browsing Mode

Added nsresult checking.
Attachment #8786971 - Attachment is obsolete: true
Attachment #8787430 - Flags: review?(jvarga)
Created attachment 8787467 [details] [diff] [review]
Patch 1 (v3) - Disable AsmJSCache in Private Browsing Mode

Added some comments along with validity check.
Attachment #8787430 - Attachment is obsolete: true
Attachment #8787430 - Flags: review?(jvarga)
Attachment #8787467 - Flags: review?(jvarga)

Comment 20

8 months ago
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(...
Attachment #8787467 - Flags: review?(jvarga) → review+

Updated

8 months ago
Blocks: 1260929

Comment 21

8 months ago
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07979189c602
Disable AsmJS caching while in Private Browsing Mode; r=janv

Comment 22

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07979189c602
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Comment 23

8 months ago
Thanks Kyle!
(Reporter)

Updated

4 months ago
Duplicate of this bug: 1326041
You need to log in before you can comment on or make changes to this bug.