Closed Bug 877924 Opened 7 years ago Closed 5 years ago

Make IDBFactory rooting less fragile

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mccr8, Assigned: jonco)

References

Details

Attachments

(1 file)

IDBFactory::Create(ContentParent* aContentParent, IDBFactory** aFactory) calls NS_HOLD_JS_OBJECTS and sets mRootedOwningObject to true, but IDBFactory::Create(JSContext* aCx, JS::Handle<JSObject*> aOwningObject, ContentParent* aContentParent, IDBFactory** aFactory) and IDBFactory::Create(nsPIDOMWindow* aWindow, const nsACString& aASCIIOrigin, ContentParent* aContentParent, IDBFactory** aFactory) set mOwningObject to something, which is what we TRACE.

It looks like we should HOLD whenever mOwningObject gets set, and remove mRootedOwningObject.  Am I missing something here?
'mOwningObject' is the object that owns the factory. In most cases we don't need to hold that object alive and 'mOwningObject' is just a weak reference. In the specific case where we have to create the owning object (a sandbox) then we have to hold it alive until we get a signal from the other process.
Thanks for the explanation!  It sounds like this isn't a problem now, but will be with moving collection.

Terrence, is there a meta bug for weak heap references to JS in the browser?
Group: core-security
Flags: needinfo?(terrence)
Summary: IDBFactory doesn't always NS_HOLD_JS_OBJECTS with mContentParent → IDBFactory has a weak JS pointer
(In reply to Andrew McCreight [:mccr8] from comment #2)
> It sounds like this isn't a problem now, but will be with moving collection.

How so? Couldn't the Trace callback do some magic to update JSObject pointers that have moved?
(In reply to ben turner [:bent] from comment #3)
> How so? Couldn't the Trace callback do some magic to update JSObject
> pointers that have moved?

Sure, that's been implemented, but the problem here is that if NS_HOLD_JS_OBJECTS hasn't been called on the IDBFactory then we won't trace it and there's no way for the GC to update it.  I guess we could add some kind of weak hold...
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Thanks for the explanation!  It sounds like this isn't a problem now, but
> will be with moving collection.
> 
> Terrence, is there a meta bug for weak heap references to JS in the browser?

No, although I probably should have.

In SpiderMonkey, we "trace" all edges, weak and strong. For strong edges we call Mark (to mark the held object as live) and for weak edges we call IsMarked/IsAboutToBeFinalized (and remove the reference if not marked). Both our Mark functions and our IsMarked/IsAboutToBeFinalized functions know how to update a moved thing. Since JS_CallFooTracer and JS_IsAboutToBeFinalized hook directly into these functions, anything that follows the same rules in the browser should be okay.

It seems like we need some sort of weak hold so that the browser can follow the same rules.
Flags: needinfo?(terrence)
Blocks: 878120
(In reply to Andrew McCreight [:mccr8] from comment #4)
> (In reply to ben turner [:bent] from comment #3)
> Sure, that's been implemented, but the problem here is that if
> NS_HOLD_JS_OBJECTS hasn't been called on the IDBFactory then we won't trace
> it and there's no way for the GC to update it.  I guess we could add some
> kind of weak hold...

Hmm, this could actually be a bit more tricky than that.

We have to be careful to trace our weak references last, since they want the mark bits for everything else to already be set correctly. We also have to iterate to a fixed point, since weakly held objects could hold other weakly held objects live transitively.
Jon is introducing a callback for exactly this purpose for compacting GC. Let's move the blocker over to Compacting GC so that this doesn't get dropped.
Assignee: nobody → jcoppeard
Blocks: 650161
No longer blocks: 878120
Blocks: 878120
No longer blocks: 878120
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #1)

First of all apologies if I didn't understand what this is going on here :)

But is this really a weak pointer?  I couldn't see any mechanism to clears the pointer if the object is finalized which would be necessary if this were possible.  Could we just always trace this pointer?
Flags: needinfo?(bent.mozilla)
jonco, I think you should probably talk to mccr8, he's more in the know on the CC machinery these days. Comment 1 is about all I know here :)
Flags: needinfo?(bent.mozilla) → needinfo?(continuation)
(In reply to Jon Coppeard (:jonco) from comment #8)
> But is this really a weak pointer?  I couldn't see any mechanism to clears
> the pointer if the object is finalized which would be necessary if this were
> possible.  Could we just always trace this pointer?

Yeah, I think you are right.  The Create() methods I mentioned in comment 0 appear to not exist any more.  So just remove mRootedOwningObject, call HoldJSObjects() in the ctor and DropJSObjects() in the dtor.
Flags: needinfo?(continuation)
Attachment #8516587 - Flags: review?(continuation)
Comment on attachment 8516587 [details] [diff] [review]
bug-877924-IDBFactory-pointer

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

r=me with the changes below

::: dom/indexedDB/IDBFactory.cpp
@@ +127,2 @@
>      mOwningObject = nullptr;
>      mozilla::DropJSObjects(this);

Make this block unconditional. With the Drop removed from Unlink (see below), you have to always Drop here because you can't tell for sure if you ever owned something or not.

@@ +718,1 @@
>      mozilla::DropJSObjects(tmp);

Just remove this Drop (and the branch on mOwningObject). For this class it is okay, but in general if somebody gets a hold of the object after it is unlinked, and manages to trigger some code that sets the pointer again, you could end up with an unrooted pointer.  So it is safer to just have a pattern where you only drop in the dtor.
Attachment #8516587 - Flags: review?(continuation) → review+
Also, just out of curiousity, is this patch really needed for moving GC?  It seems like as long as we maintain the fragile invariant that mRootedOwningObject is monotonic and is always true when we set the JS pointer, it should be okay.
Summary: IDBFactory has a weak JS pointer → Make IDBFactory rooting less fragile
(In reply to Andrew McCreight [:mccr8] from comment #14)
Thanks for the comments.

If mRootedOwningObject is always true when we have a JS pointer then it's fine.  But it does look fragile so it's better to simplify this to be on the safe side.
https://hg.mozilla.org/mozilla-central/rev/8aab8aebc1a5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.