1117172 - Figure out how to properly set the prototype for subclassed DOM objects

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We need to figure out how to deal with properly setting the prototype for subclassed DOM objects.

The current setup we have for DOM constructors is that the constructor (the JSClass construct hook) allocates the right C++ object and then invokes a virtual method (WrapObject) on it to create the appropriate JSObject.  This virtual method calls into the right generated binding code, where "right" may be determined by the constructor arguments.  This allows a good bit of flexibility for the C++ implementation; for example you can have classes A and B both inherit from C, implement the virtual method on C, but call into the generated code for A or B depending on which one we were actually constructed as. The generated code statically bakes in the prototype to use.

This setup is due to sharing the same codepath for creating the JS object whether the C++ object was created via the JS constructor or browser-internal code (possibly long before we actually needed the JS reflection).

For ES6 subclassing, we will need to dynamically determine the prototype based on which constructor function was actually invoked to start with.  We will presumably have that information in the construct hook, since the spec's [[Construct]] will grow some way to pass this information along.  But per above, the actual object allocation happens in the generated object-creation code, which doesn't have that argument.  Furthermore, in the non-JS-constructor case the place that needs to create the JS reflection may not even know what proto is desired; for example the "firstChild" getter just knows it has a Node; which exact one is not known at that point, and getting into the right generated JSObject-creation code is handled by the combination of virtual dispatch of WrapObject and whatever introspection the WrapObject implementation then does.

I think we have the following possible implementation strategies:

1)  Leave the WrapObject setup as is.  Have the construct hook allocate the new object, then mutate its proto to the correct one.  Pro: minimal DOM changes needed.  Con: needs a way to mutate the proto without deoptimizing, akin to what JS_SplicePrototype does, but for non-singletons; this would presumably give the object a new Type and whatnot.  Note that we're guaranteed that at this point the object has not been exposed to script yet, at least, so that's good.

2)  Change the signature of WrapObject to take a proto argument, with null meaning "use the default proto".  This requires a mass-change to the 300-some WrapObject imlementations, but it's likely this could be mostly scripted.  Adds a branch (for the null-check) to the allocation codepath, but that seems acceptable performance-wise.

3)  Add a WrapObjectWithGivenProto that bindings expect to be implemented on any DOM class that has a constructor.  This would need to be virtual (e.g. to handle the case of |new HTMLElement("img")| if that ever comes up, and we'd need to make sure that any subclasses of the class bindings know about that override WrapObject also override WrapObjectWithGivenProto.  Looks like we'd need to add this to about 180 classes, which is almost worse than option #2, since this would be harder to automate.

In all cases we'd need to preserve the object's wrapper if a non-default proto is used for a wrapper-cached object, of course.

Now that I've written all this out, my gut feeling is that we should do #2, I think.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Brian, just to check, how viable is option #1 in practice?

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Brian, just to check, how viable is option #1 in practice?

This would be fine to do so long as the object hasn't escaped to script.  If any properties have been added to the object at the point its prototype changes, we'd need to make sure that type information is reflected in the object's new type, and if the object has been stored to locations tracked by TI (e.g. properties of other objects) then the type information for those locations would need to be updated as well.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We can guarantee not escaping to script, but perhaps only modulo bug 905168.

Properties might have been added to the object, though, due to unforgeable properties in the IDL.  And guaranteeing that some of those didn't stash this object in a slot is not trivial...

Sounds like we should do #2, then.  It's the most straightforward approach.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Allow passing an optional aGivenProto to binding Wrap methods

Peter, please let me know if you prefer a name other than aGivenProto?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change the non-wrappercached WrapObject methods to allow passing in aGivenProto

The only manual changes here are to BindingUtils.h, Codegen.py, and
StructuredClone.cpp.  The rest of this diff was generated by running the following commands:

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/WrapObject\((JSContext *\* *(?:aCx|cx)),(\s*)(JS::MutableHandle<JSObject\*> aReflector)/WrapObject(\1,\2JS::Handle<JSObject*> aGivenProto,\2\3/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(Binding(?:_workers)?::Wrap\((?:aCx|cx)), this, aReflector/\1, this, aGivenProto, aReflector/'

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change the wrappercached WrapObject methosd to allow passing in aGivenProto

The only manual changes here are to BindingUtils.h, BindingUtils.cpp,
Codegen.py, Element.cpp, IDBFileRequest.cpp, IDBObjectStore.cpp,
dom/workers/Navigator.cpp, WorkerPrivate.cpp, DeviceStorageRequestChild.cpp,
Notification.cpp, nsGlobalWindow.cpp, MessagePort.cpp, nsJSEnvironment.cpp,
Sandbox.cpp, XPCConvert.cpp, and DataStoreService.cpp.  The rest of this diff
was generated by running the following commands:

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(WrapObjectInternal\(JSContext *\* *(?:aCx|cx|aContext|aCtx|js))\)/\1, JS::Handle<JSObject*> aGivenProto)/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(WrapObjectInternal\((?:aCx|cx|aContext|aCtx|js))\)/\1, aGivenProto)/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(WrapNode\(JSContext *\* *(?:aCx|cx|aContext|aCtx|js))\)/\1, JS::Handle<JSObject*> aGivenProto)/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(WrapNode\((?:aCx|cx|aContext|aCtx|js))\)/\1, aGivenProto)/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(WrapObject\(JSContext *\* *(?:aCx|cx|aContext|aCtx|js))\)/\1, JS::Handle<JSObject*> aGivenProto)/g'

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef } s/(Binding(?:_workers)?::Wrap\((?:aCx|cx|aContext|aCtx|js), [^,)]+)\)/\1, aGivenProto)/g'

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Use the new proto setup for custom element prototypes

Comment 8

[Boris Zbarsky [:bzbarsky]]

Some notes:

1)  The only substantive manual changes in part 3 are in BindingUtils.h, BindingUtils.cpp,
Codegen.py, and maybe Element.cpp.  The rest are adding NullPtr() to WrapObject calls.  We could avoid those if we make the WrapObject argument optional.  I'm glad I didn't so far, since it found callsites like Element::WrapObject that I would have missed otherwise, but now that we have the patch in its current form, that would let us drop some of the manual changes if we want to.

2)  I'm not worrying about passing anything other than nullptr from bindings so far.  That part will get fixed in bug 1135961.  Would like to land this part first, before it bitrots.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  There is one test failure due to the assert I put into Wrap about aCache->GetWrapper() being null.  It's failing for DataStore because DataStoreService::GetDataStoresResolve does the following:

1)  Create a DataStore.
2)  Call SetDataStoreImpl on it, which calls SetEventTarget on the DataStoreImpl with the
    DataStore as argument, which wraps the DataStore, because DataStoreImpl is
    JS-implemented.
3)  Calls WrapObject on the DataStore.

I'm going to loosen up the assert for now to only assert if aGivenProto and retighten it up in bug 1143529.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Allow passing an optional aGivenProto to binding Wrap methods

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Allow passing an optional aGivenProto to binding Wrap methods

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8577848 [details] [diff] [review]
part 1.  Allow passing an optional aGivenProto to binding Wrap methods

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

::: dom/bindings/Codegen.py
@@ +3183,5 @@
>      copyCode.append(CGGeneric(fill(
>          """
>          // XXXbz Once we allow subclassing, we'll want to make sure that
>          // this uses the canonical proto, not whatever random passed-in
>          // proto we end up using for the object.

Maybe remove the comment now?

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8577565 [details] [diff] [review]
part 2.  Change the non-wrappercached WrapObject methods to allow passing in aGivenProto

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

A little sad about the long lines, but oh well.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8577566 [details] [diff] [review]
part 3.  Change the wrappercached WrapObject methosd to allow passing in aGivenProto

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

I wonder if there's something we could do to hide the aGivenProto for all those callers that don't use it (instead of forcing them to pass in NullPtr()). An one-argument inline WrapObject on nsWrapperCache that forwards with a NullPtr()? Not sure it'd work.

Any chance you could replace cx with aCx in the lines that you touched?
And yeah, long lines, sad, oh well.

::: dom/base/nsINode.h
@@ +379,5 @@
>    /**
>     * WrapNode is called from WrapObject to actually wrap this node, WrapObject
>     * does some additional checks and fix-up that's common to all nodes. WrapNode
>     * should just call the DOM binding's Wrap function.
>     */

Please document the aGivenProto member.

::: dom/svg/DOMSVGPathSeg.h
@@ +64,2 @@
>    {                                                                           \
> +    return dom::SVGPathSeg##segName##Binding::Wrap(aCx, this, aGivenProto);        \

I think we should line up the \'s here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I wonder if there's something we could do to hide the aGivenProto for all those callers
> that don't use it

We could make it optional; see comment 8.

> Any chance you could replace cx with aCx in the lines that you touched?

I was really trying to not change stuff around in the interest of not introducing bugs... I can try to make those changes if you'd really like, I guess.

> Please document the aGivenProto member.

Will do.

> I think we should line up the \'s here.

Will do.

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to Not doing reviews right now from comment #15)
> > I wonder if there's something we could do to hide the aGivenProto for all those callers
> > that don't use it
> 
> We could make it optional; see comment 8.

If we can't limit the change to nsWrapperCache and any callers then nevermind.

> > Any chance you could replace cx with aCx in the lines that you touched?
> 
> I was really trying to not change stuff around in the interest of not
> introducing bugs... I can try to make those changes if you'd really like, I
> guess.

Up to you, I don't feel strongly about it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Maybe remove the comment now?

Yes, good catch.

> Please document the aGivenProto member.

Done.

> I think we should line up the \'s here.

Done.

Part 4 has gotten bitrotted by bug 1130028, so I'm going to spin it out into a separate bug 1145017, since it was really opportunistic here anyway.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eeccb62c773
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2f419d98ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0c492a33b5

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/5eeccb62c773
https://hg.mozilla.org/mozilla-central/rev/9b2f419d98ab
https://hg.mozilla.org/mozilla-central/rev/0b0c492a33b5