1176341 - De-holder nsIXPConnect::CreateSandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This can't be done easily because CompareManager (in ServiceWorkerScriptCache) appears to use the JS holder returned by GetSandbox to keep alive the sandbox from an object off the main thread. This is similar to what some storage code does, so maybe a method should be added to xpcpublic that creates a holder for use in the few places where we need it.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::CreateSandbox. WIP

This is the start of a patch.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::CreateSandbox, except Console.

This patch makes CompareManager use a PersistentRooted. I think all of the methods for this class assert that they are on the main thread (more or less) so maybe this is only used from the main thread? This does not deal with Console.

Comment 3

[Andrew McCreight [:mccr8]]

Andrea, how is the threadedness of Console supposed to work? I don't entirely understand. It holds a pointer to a sandbox, which is a main-thread thing, but it can be used on a worker thread? And it just promises to never touch that sandbox pointer unless it is on the main thread? How is it okay for a Console object that is being used on the worker thread ever get called on the main thread? That should cause AddRef/Release thread assertions, but maybe there is care taken so only one thread at a time can touch it and the main thread won't ever change the refcount? Sorry for the rambling comment.

Comment 4

[Andrea Marchesini [:baku]]

The console implementation works in this way:

1. on the main-thread we have a console obj stored in the window object and this has directly access to all the main-thread things.

2. on a 'normal' worker, we have a console that uses runnables to do operations on the main-thread. The window object is used.

3. for window-less workers (sharedWorkers, chromeWorkers, serviceWorkers) we create and use a sandbox on the main-thread only.

About refcounting, Console does addRef/Release only in its own thread. For instance ConsoleRunnable is created in a worker and it keeps a reference to the console. Once the main-thread operation is done, it uses a ConsoleReleaseRunnable to release the console obj, again in the worker thread.

Comment 5

[Andrew McCreight [:mccr8]]

Thanks for the explanation. It sounds like Console really does need some kind of "keep a JS object alive on thread A from thread B" thing. We need that in a few other places, too.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::CreateSandbox.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::CreateSandbox.

r?baku for the DOM changes, gabor for the xpconnect changes

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d3af32a091

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8630149 [details] [diff] [review]
De-holder nsIXPConnect::CreateSandbox.

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

All the non-js files are ok to me.

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8630149 [details] [diff] [review]
De-holder nsIXPConnect::CreateSandbox.

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

::: dom/base/Console.h
@@ +8,5 @@
>  #define mozilla_dom_Console_h
>  
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/JSObjectHolder.h"

Have you forgotten to add this file to the patch? Or is someone else reviewing that file in another patch somewhere? I would r+ the patch but would like to see this file first just in case...

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +619,5 @@
>      cachePromise->AppendNativeHandler(this);
>    }
>  
>    nsRefPtr<CompareCallback> mCallback;
> +  JS::PersistentRooted<JSObject*> mSandbox;

I don't know anything about CompareManager. Are you sure that it does not have a JSObject somewhere on its ownership chain? If it's life time depended on a JSObject then this would be a leak but I cannot tell that, could you double check that? (if you haven't already ofc...) Maybe a short warning in the comment makes sense too...

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> Have you forgotten to add this file to the patch? Or is someone else
> reviewing that file in another patch somewhere? I would r+ the patch but
> would like to see this file first just in case...
Sorry, I should have mentioned that JSObjectHolder is being added in bug 1179924, which this bug depends on. I hadn't bothered landing it because this is the first use of it.

> I don't know anything about CompareManager. Are you sure that it does not
> have a JSObject somewhere on its ownership chain? If it's life time depended
> on a JSObject then this would be a leak but I cannot tell that, could you
> double check that? (if you haven't already ofc...) Maybe a short warning in
> the comment makes sense too...

Yeah, it is pretty weird. I don't really understand what it is supposed to be doing. I will note that my patch does not make the situation any worse. ;) I'll try tracking down somebody who knows what is going on with it.

(FWIW, I was only asking for review from you on the XPConnect stuff, though of course comments are welcome on the entire patch.)

Comment 11

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8630149 [details] [diff] [review]
De-holder nsIXPConnect::CreateSandbox.

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

(In reply to Andrew McCreight [:mccr8] from comment #10)
> ;) I'll try tracking down somebody who knows what is going on with it.
> 
> (FWIW, I was only asking for review from you on the XPConnect stuff, though
> of course comments are welcome on the entire patch.)

Makes sense, I'm totally OK with not having to review lifetime management of
those objects :) r+ for the xpconnect related changes.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74293125739a

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b35af146ed92 for random frequent serviceworker test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=11567906&repo=mozilla-inbound

Comment 14

[Andrew McCreight [:mccr8]]

From the error, it looks like I managed to break part 2 of bug 1154494 somehow.

Comment 15

[Andrew McCreight [:mccr8]]

nsm, what exactly is the deal with this: https://hg.mozilla.org/mozilla-central/rev/1a39f8cf4130

I don't really understand. Is the problem that the sandbox would go away while the CompareManager is still alive?  I tried changing it so that instead of the CompareManager holding onto the nsIXPConnectJSObjectHolder it would directly hold onto the JSObject that is the wrapper, but that seemed to bring back the symptoms you needed part 2 of bug 1154494 for. I also tried a variant that instead had the CompareManager hold onto the nsIGlobalObject created by the NativeGlobal call in CreateCacheStorage, but that didn't seem to work either [1]. Do you have any idea why either of those would fail to work?

[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=03d7b5a6d8c0

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrew did the IRC conversation help? If not, please ni? again.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #16)
> Andrew did the IRC conversation help? If not, please ni? again.

I haven't had a chance to look at it yet. I should have just cleared the ni.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::CreateSandbox.

I figured out why my previous patch wasn't working: I wasn't
initializing the PersistentRooted when storing something in it, so the
sandbox reflector was going away. That was not causing an assertion
failure due to a bug in PersistentRooted which I have a patch for
elsewhere.

Andrea, could you please review my changes to ServiceWorkerScriptCache?
The rest is the same. I added a comment explaining what is going on with
the reflector object rooting. I also made the handle object mandatory,
to make it more explicit that the lifetimes of the CacheStorage and reflector
objects should be linked. Frankly, I'm not sure how the other caller of
CreateCacheStorage works, but if anybody starts noticing intermittent
failures, this is probably the reason.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8636610 [details] [diff] [review]
De-holder nsIXPConnect::CreateSandbox.

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

I didn't know about JS:PersistentRooted. Looks good to me.

Comment 20

[Andrew McCreight [:mccr8]]

Here's my try run that demonstrates that the new version of the patch doesn't cause the e10s ServiceWorker tests to fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e81568372d81

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/553a3e1e7b18

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=11953718&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/a86a6429b078

Comment 23

[Andrew McCreight [:mccr8]]

Oops, I forgot to land the dependent bug (which I had applied in my local repo) at the same time!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11ee37ed0cd9

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11ee37ed0cd9