This patch creates API that allows one to manually pass in a scope object to the typed array creation code (rather than rely on it grabbing it from a nsWrapperCache*) and a WebIDL annotation that passes in the 'this' object. It also converts TextEncoder to use this. Later we will remove the wrapper caching from TextEncoder.

Codegen doesn't know what to do with ctors for owned objects. resultAlreadyAddrefed is true because ownership is being transferred here, but the underlying object is not refcounted. Longer term we probably want to change resultAlreadyAddrefed to something like 'transfersOwnership'

Comment on attachment 788536 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* Review of attachment 788536 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +70,5 @@ > # * implicitJSContext - attributes and methods specified in the .webidl file > # that require a JSContext as the first argument > +# * implicitScopeObject - attributes and methods specified in the .webidl > +# file that require the scope object as the second > +# argument. Must be specified with implicitJSContext. This last part doesn't seem enforced? ::: dom/encoding/TextEncoderBase.h @@ +63,4 @@ > > protected: > + JSObject* > + CreateUint8Array(JSContext* aCx, JS::Handle<JSObject*> aObj, Trailing ws

WorkerGlobalObject is really thread-agnostic. This patch renames the existing GlobalObject to MainThreadOnlyGlobalObject, and renames WorkerGlobalObject to GlobalObject. MainThreadOnlyGlobalObject inherits from GlobalObject and includes the unwrap to nsISupports* code that runs lazily. The codegen always passes in a MainThreadOnlyGlobalObject, but code that wants to work on both threads can limit itself to accepting a GlobalObject.

This implements a per-runtime atom cache hanging off the atom compartment that contains the jsids necessary for dictionaries. We have a struct of jsids for each dictionary, and a big struct that inherits from all the little structs. Initializing the jsids in reverse order allows us to see if we've already inited a dict by just comparing the first jsid to nullptr. Once the atom situation is sorted we can use the same dictionary implementation on both threads.

This removes the now unnecessary worker thread TextDecoder implementation and also wrappercaching which I don't think was ever needed (since TextDecoder can only ever be owned by JS).

Do you know anything we could replace TextDecoder with here? I didn't see anything offhand (although we should be able to remove the nsISupports inheritance from ImageData at which point it could work here).

Comment on attachment 788536 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* The CGNativeMember bits don't look right in cases when there is an implicitScopeObject but no cx... I think we should insert the obj at 0, but just do it before maybe-inserting cx. The other concern I have is that if this is used to create objects that are then stored in the c++ and if the first call to the C++ to get the object is via an xray, then we'll end up storing the object created in the Xray compartment, which is a bit different from what we do right now.

Comment on attachment 788539 [details] [diff] [review] Part 4: Harmonize worker and main thread dictionary implementations I suspect we no longer need the split between DictionaryBase and MainThreadDictionaryBase.

Comment on attachment 788540 [details] [diff] [review] Part 5: Remove the worker thread TextDecoder impl and wrapper caching \o/ ::: dom/encoding/TextDecoder.h > private: nit: This line is redundant now. Same for the TextEncoder.h.

Comment on attachment 788541 [details] [diff] [review] Part 6: Remove TextDecoderBase ::: content/base/src/nsContentUtils.cpp >+ nsAutoPtr<TextDecoder> decoder = new TextDecoder(); >+ decoder->Init(NS_ConvertUTF8toUTF16(aCharset), false, rv); How about TextDecoder decoder; decoder.Init(...); ? Do we have to bothered with the pointer here?

Comment on attachment 788543 [details] [diff] [review] Part 8: Remove TextEncoderBase ::: dom/encoding/TextEncoder.h >+protected: >+ JSObject* >+ CreateUint8Array(JSContext* aCx, JS::Handle<JSObject*> aObj, >+ char* aBuf, uint32_t aLen) const >+ { >+ return Uint8Array::Create(aCx, aObj, aLen, >+ reinterpret_cast<uint8_t*>(aBuf)); > } Please remove this method and call Uint8Array::Create() directly. I had to add this method because two subclasses used to use two different implementations. It no longer holds.

(In reply to Vacation until Aug 19. Do not ask for review. from comment #11) > Comment on attachment 788536 [details] [diff] [review] > Part 1: Add an API for creating TypedArrays that does not require a > nsWrapperCache* > > The CGNativeMember bits don't look right in cases when there is an > implicitScopeObject but no cx... I think we should insert the obj at 0, but > just do it before maybe-inserting cx. I should add the bit that throws if you don't do implicitJSContext too also. > The other concern I have is that if this is used to create objects that are > then stored in the c++ and if the first call to the C++ to get the object is > via an xray, then we'll end up storing the object created in the Xray > compartment, which is a bit different from what we do right now. Ugh. *@$# xrays.

Comment on attachment 788536 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* Review of attachment 788536 [details] [diff] [review]: ----------------------------------------------------------------- Can we avoid the implicitScopeObject for now by automatically passing in the additional argument if the return value is a typed array? I don't think we have many APIs that do that yet, so it shouldn't require too many changes.

Comment on attachment 788537 [details] [diff] [review] Part 2: Fix handing of ctor return values Review of attachment 788537 [details] [diff] [review]: ----------------------------------------------------------------- I ran into this too.

(In reply to Peter Van der Beken [:peterv] from comment #17) > Comment on attachment 788536 [details] [diff] [review] > Part 1: Add an API for creating TypedArrays that does not require a > nsWrapperCache* > > Review of attachment 788536 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can we avoid the implicitScopeObject for now by automatically passing in the > additional argument if the return value is a typed array? I don't think we > have many APIs that do that yet, so it shouldn't require too many changes. And it would be better if the typed array return value also implies 'implicitJSContext'. We already do that for 'any' (or JS::Value) return values.

(In reply to Peter Van der Beken [:peterv] from comment #17) > Comment on attachment 788536 [details] [diff] [review] > Part 1: Add an API for creating TypedArrays that does not require a > nsWrapperCache* > > Review of attachment 788536 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can we avoid the implicitScopeObject for now by automatically passing in the > additional argument if the return value is a typed array? I don't think we > have many APIs that do that yet, so it shouldn't require too many changes. My concern about doing that is that it's only needed if your object is not a wrapper cache. We could implement that ... but I'd rather get this into the tree and do that in a followup (so we can unblock smaug). (In reply to Masatoshi Kimura [:emk] from comment #19) > (In reply to Peter Van der Beken [:peterv] from comment #17) > > Comment on attachment 788536 [details] [diff] [review] > > Part 1: Add an API for creating TypedArrays that does not require a > > nsWrapperCache* > > > > Review of attachment 788536 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Can we avoid the implicitScopeObject for now by automatically passing in the > > additional argument if the return value is a typed array? I don't think we > > have many APIs that do that yet, so it shouldn't require too many changes. > > And it would be better if the typed array return value also implies > 'implicitJSContext'. > We already do that for 'any' (or JS::Value) return values. Agreed, but I also want to do that in a followup.

> My concern about doing that is that it's only needed if your object is not a > wrapper cache. We could implement that ... but I'd rather get this into the > tree and do that in a followup (so we can unblock smaug). That's fine with me if the followup lands before the next merge, we've been removing annotations as much as possible and I'd rather not add more just to remove them again later. > > And it would be better if the typed array return value also implies > > 'implicitJSContext'. > > We already do that for 'any' (or JS::Value) return values. > > Agreed, but I also want to do that in a followup. That already works afaik, the 'implicitJSContext' for 'encode' just hasn't been removed.

Comment on attachment 788539 [details] [diff] [review] Part 4: Harmonize worker and main thread dictionary implementations Review of attachment 788539 [details] [diff] [review]: ----------------------------------------------------------------- Note sure that the auto's are very useful. ::: dom/bindings/Codegen.py @@ +8070,3 @@ > (m.identifier.name + "_id", m.identifier.name)) > for m in self.dictionary.members] > + idinit.reverse(); Make a comment that you reverse so that any failure leaves the first id uninitialized.

Comment on attachment 788544 [details] [diff] [review] Part 9: Disable a test that relied on TextDecoder being cycle collected Review of attachment 788544 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, there are still plenty of non-nsISupports refcounted and CC'ed classes, no? (Look for NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE) RGBColor looks like a candidate (window.getComputedStyle(document.documentElement, null).getPropertyCSSValue("color").getRGBColorValue()).

Comment on attachment 788536 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* Review of attachment 788536 [details] [diff] [review]: ----------------------------------------------------------------- Minusing for the issues signaled in comment 11, we really should unwrap the object passed in. Also, as said in comment 21, I'm ok with temporarily landing but I really want that Bindings.conf annotation gone as soon as possible and before the next merge at the latest.

Comment on attachment 788538 [details] [diff] [review] Part 3: Refactor GlobalObject Review of attachment 788538 [details] [diff] [review]: ----------------------------------------------------------------- As discussed, let's collapse MainThreadOnlyGlobalObject into GlobalObject.

Your silly insistence on correctness made this much more complicated.

This probably shows some artifacts of renaming everything twice.

Comment on attachment 793358 [details] [diff] [review] Part 3: Refactor GlobalObject Review of attachment 793358 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: content/base/public/nsIDocument.h @@ +101,3 @@ > class HTMLBodyElement; > class Link; > +class GlobalObject; Undo this. ::: content/media/webaudio/AudioContext.h @@ +52,3 @@ > class HTMLMediaElement; > class MediaElementAudioSourceNode; > +class GlobalObject; Undo this. ::: dom/encoding/TextDecoder.h @@ +20,4 @@ > > // The WebIDL constructor. > static already_AddRefed<TextDecoder> > + Constructor(const MainThreadOnlyGlobalObject& aGlobal, Uhoh. Please compile. ::: dom/encoding/TextEncoder.h @@ +20,4 @@ > > // The WebIDL constructor. > static already_AddRefed<TextEncoder> > + Constructor(const MainThreadOnlyGlobalObject& aGlobal, And here.

Comment on attachment 793357 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* Review of attachment 793357 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +4694,5 @@ > + needsUnwrap = isConstructor > + if needScopeObject(returnType, arguments, self.extendedAttributes, > + descriptor, descriptor.wrapperCache, > + not descriptor.interface.isJSImplemented()): > + cgThings.append(CGGeneric("Maybe<JS::Rooted<JSObject*> > unwrappedObj;")) Can you add a comment that we're doing this because obj is a Handle<...> and so we can't assign the result of CheckedUnwrap to it? @@ +4728,5 @@ > + xraySteps = [] > + if not isConstructor: > + xraySteps.append( > + CGGeneric("unwrappedObj.construct(cx, obj);\n" > + "JS::Rooted<JSObject*>& obj = unwrappedObj.ref();")) The shadowing of 'obj' makes me cringe a bit (well, this whole thing but I don't see a good way around it). Could we just fill in |obj| or |unwrappedObj.ref()| in the line where we set it to |js::CheckedUnwrap(obj)| depending on the value of isConstructor? @@ +8931,5 @@ > args.insert(0, Argument("JSContext*", "cx")) > + if needScopeObject(returnType, argList, self.extendedAttrs, > + self.descriptorProvider, self.descriptorProvider, > + self.passJSBitsAsNeeded): > + args.insert(0, Argument("JS::Handle<JSObject*>", "obj")) This should happen before we insert the cx, no? ::: dom/bindings/TypedArray.h @@ +105,5 @@ > if (creator && (creatorWrapper = creator->GetWrapperPreserveColor())) { > ac.construct(cx, creatorWrapper); > } > + > + return CreateCommon(cx, creatorWrapper, length, data); I don't think you actually need CreateCommon. AFAICT this one should just be: return Create(cx, creator ? creator->GetWrapperPreserveColor() : nullptr, length, data); @@ +116,5 @@ > + if (creator) { > + ac.construct(cx, creator); > + } > + > + return CreateCommon(cx, creator, length, data); and you can replace this with the implementation of CreateCommon. ::: dom/encoding/TextEncoderBase.h @@ +63,3 @@ > > protected: > + JSObject* static? @@ +63,4 @@ > > protected: > + JSObject* > + CreateUint8Array(JSContext* aCx, JS::Handle<JSObject*> aObj, Trailing whitespace.

Comment on attachment 793357 [details] [diff] [review] Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache* ::: dom/bindings/Configuration.py >- for attribute in ['implicitJSContext', 'resultNotAddRefed']: >+ for attribute in ['implicitJSContext', 'implicitScopeObject', 'resultNotAddRefed']: Is this still needed?

(In reply to Masatoshi Kimura [:emk] from comment #30) > Comment on attachment 793357 [details] [diff] [review] > Part 1: Add an API for creating TypedArrays that does not require a > nsWrapperCache* > > ::: dom/bindings/Configuration.py > >- for attribute in ['implicitJSContext', 'resultNotAddRefed']: > >+ for attribute in ['implicitJSContext', 'implicitScopeObject', 'resultNotAddRefed']: > > Is this still needed? No, it's not. Good catch.

https://hg.mozilla.org/mozilla-central/rev/2df0b1968e70 https://hg.mozilla.org/mozilla-central/rev/910171cdb49a https://hg.mozilla.org/mozilla-central/rev/8747c1028f1c https://hg.mozilla.org/mozilla-central/rev/755b0e538b7a https://hg.mozilla.org/mozilla-central/rev/94d0b33d4d8a https://hg.mozilla.org/mozilla-central/rev/dabbf6275092 https://hg.mozilla.org/mozilla-central/rev/3d866e6ca83a https://hg.mozilla.org/mozilla-central/rev/27ba8b41c458 https://hg.mozilla.org/mozilla-central/rev/e1bd096607b1

> https://hg.mozilla.org/mozilla-central/rev/94d0b33d4d8a > https://hg.mozilla.org/mozilla-central/rev/dabbf6275092 > https://hg.mozilla.org/mozilla-central/rev/3d866e6ca83a > https://hg.mozilla.org/mozilla-central/rev/27ba8b41c458 Looks like my review comments are not reflected. Please explain the reason if this is intentional, at least.

(In reply to Masatoshi Kimura [:emk] from comment #33) > > https://hg.mozilla.org/mozilla-central/rev/94d0b33d4d8a > > https://hg.mozilla.org/mozilla-central/rev/dabbf6275092 > > https://hg.mozilla.org/mozilla-central/rev/3d866e6ca83a > > https://hg.mozilla.org/mozilla-central/rev/27ba8b41c458 > > Looks like my review comments are not reflected. > Please explain the reason if this is intentional, at least. I missed the comment on part 8. I pushed a followup to fix it: https://hg.mozilla.org/integration/mozilla-inbound/rev/922e1a74789f I decided not to do the comment on part 6 because stack allocating DOM objects scares me a bit. If the DOM object changes in the future (like to become refcounted or cycle collected) it could cause problems.

So after these changes, do we still need the MainThreadDictionaryBase/DictionaryBase distinction? Also, it looks like we now codegen all dictionaries, so we can remove the dictionary bits from DummyBinding, right?

Yeah I suspect we can consolidate them.

Alright. I filed bug 968665 on doing that.