1478306 - CacheCreator::CreateCacheStorage does not use the sandbox it creates?

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

We do this [0]:

  mozilla::AutoSafeJSContext cx;
  JS::Rooted<JSObject*> sandbox(cx);
  nsresult rv = xpc->CreateSandbox(cx, aPrincipal, sandbox.address());
  ...
  mSandboxGlobalObject = xpc::NativeGlobal(sandbox);

AutoSafeJSContext is basically AutoJSAPI initialized to the UnprivilegedJunkScope() [1]. Because we're in a compartment, CreateSandbox will then wrap the return value in that compartment.

So unless I'm missing something, mSandboxGlobalObject here is actually the unprivileged junk scope's global.

The service worker code looks similar, but there we don't use AutoSafeJSContext but an AutoJSAPI with a null realm/compartment, so I think that one is correct.

The service worker code does have this scary comment:

// XXX A sandbox nsIGlobalObject does not preserve its reflector, so |aSandbox|
// must be kept alive as long as the CacheStorage if you want to ensure that
// the CacheStorage will continue to work. 

So that means fixing CacheCreator::CreateCacheStorage is probably not entirely trivial.

[0] https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/workers/ScriptLoader.cpp#1517-1524

[1] https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/script/ScriptSettings.cpp#806

[2] https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/serviceworkers/ServiceWorkerScriptCache.cpp#63

Comment 1

[Jan de Mooij [:jandem]]

baku, do you know what the implications of this are? It's not observable right? Any chance you could take this?

It's the last blocker for bug 1474272.

Comment 2

[Jan de Mooij [:jandem]]

A Try push confirms |sandbox| is not a global object so it must be a cross-compartment wrapper.

We should either nuke the unused sandbox and just use the UnprivilegedJunkScope explicitly, or fix the code to use the sandbox. I don't know why the code wanted to use a sandbox though...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> but an AutoJSAPI with a null realm/compartment

I think that's what we should do here, assuming we really want a separate sandbox.

> // XXX A sandbox nsIGlobalObject does not preserve its reflector

That changed in bug 1146316.  See SandboxPrivate::Create.  I filed bug 1478390 to clean up the service worker code and remove that comment.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> > // XXX A sandbox nsIGlobalObject does not preserve its reflector
> 
> That changed in bug 1146316.

Oh, excellent. That explains why the patch I wrote for this was green!

I can probably fix this myself then.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Actually use the sandbox we create in CacheCreator::CreateCacheStorage

There's still the question of do we *want* the sandbox, but this at least makes the code do what was intended.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994879 [details] [diff] [review]
Actually use the sandbox we create in CacheCreator::CreateCacheStorage

I'd really like Andrea  to take a look at this.

Comment 7

[Jan de Mooij [:jandem]]

baku, review ping? This is the last blocker for bug 1474272, which is the last blocker for bug 1472973. I'm a bit worried things will regress if I don't get these bugs landed soonish.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8994879 [details] [diff] [review]
Actually use the sandbox we create in CacheCreator::CreateCacheStorage

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

Looks good. Thanks! The sandbox is still needed to have a global for the cache API, when the SW stores the script in the cache db.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40bdd403568e
Actually use the sandbox we create in CacheCreator::CreateCacheStorage. r=baku

Comment 10

[Jan de Mooij [:jandem]]

Thanks for the review!

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/40bdd403568e