903772 - Port the main thread TextDecoder/Encoder implementation to workers.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache*

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.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Fix handing of ctor return values

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 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Refactor GlobalObject

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 4: Harmonize worker and main thread dictionary implementations

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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 5: Remove the worker thread TextDecoder impl and wrapper caching

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).

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 6: Remove TextDecoderBase

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 7: Remove the worker thread TextEncoder impl and wrapper caching

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 8: Remove TextEncoderBase

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 9: Disable a test that relied on TextDecoder being cycle collected

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 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Masatoshi Kimura [:emk]]

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 14

[Masatoshi Kimura [:emk]]

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 15

[Masatoshi Kimura [:emk]]

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.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 17

[Peter Van der Beken [:peterv]]

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 18

[Peter Van der Beken [:peterv]]

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.

Comment 19

[Masatoshi Kimura [:emk]]

(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.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 21

[Peter Van der Beken [:peterv]]

> 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 22

[Peter Van der Beken [:peterv]]

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 23

[Peter Van der Beken [:peterv]]

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 24

[Peter Van der Beken [:peterv]]

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 25

[Peter Van der Beken [:peterv]]

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.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Add an API for creating TypedArrays that does not require a nsWrapperCache*

Your silly insistence on correctness made this much more complicated.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Refactor GlobalObject

This probably shows some artifacts of renaming everything twice.

Comment 28

[Peter Van der Beken [:peterv]]

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 29

[Peter Van der Beken [:peterv]]

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 30

[Masatoshi Kimura [:emk]]

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?

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Masatoshi Kimura [:emk]]

> 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.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/922e1a74789f

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Add a license header to AtomList.h

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2faa6e031add

Comment 38

[Boris Zbarsky [:bzbarsky]]

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?

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah I suspect we can consolidate them.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Alright.  I filed bug 968665 on doing that.