919885 - Move Worker to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

Move the actual Worker object (not the worker global) to use nsDOMEventTargetHelper and WebIDL.

Comment 1

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

Attachment: Patch

Worker is complex because the underlying C++ object is held alive from two threads (although all reference counting happens on the parent thread).  We may want to talk about this in Toronto this weekend.

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 813004 [details] [diff] [review]
Patch

::: dom/bindings/Bindings.conf
@@ -26,16 +26,21 @@
>+#   * wantsQI - Indicates whether the interface should have a QueryInterface
>+#               method available to chrome.

wantsQI was removed by bug 920196.

::: dom/bindings/Configuration.py
@@ -351,22 +352,27 @@ class Descriptor(DescriptorProvider):
>         if desc.get('wantsQI', None) != None:
>             self._wantsQI = desc.get('wantsQI', None)

Looks like this is (perhaps accidentally) added back in bug 919457.

Comment 3

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

Comment on attachment 813004 [details] [diff] [review]
Patch

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

::: dom/workers/RuntimeService.cpp
@@ +996,4 @@
>      return true;
>    }
>  
> +  WorkerBinding::GetConstructorObject(aCx, aObj);

Can these fail?

::: dom/workers/WorkerPrivate.cpp
@@ +2642,5 @@
> +bool
> +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> +{
> +  // Chrome is always allowed to use workers, and content is never allowed to
> +  // use ChromeWorker, so all we have to check 

Trailing ws

@@ +4545,5 @@
>  
>  BEGIN_WORKERS_NAMESPACE
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)

const JS::Value&

@@ +4547,5 @@
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> +{
> +  if (JSVAL_IS_PRIMITIVE(aWorker)) {

!aWorker.isObject(), please

::: dom/workers/WorkerPrivate.h
@@ +979,5 @@
>  };
>  
> +// This class is only used to trick the DOM bindings.  We never create
> +// instances of it, and static_casting to it is fine since it doesn't add
> +// anything to WorkerPrivate.

:/

::: dom/workers/WorkerScope.cpp
@@ +1119,5 @@
>    }
>  
>    // Init other paris-bindings.
> +  if (!(!worker->IsChromeWorker() ||
> +        ChromeWorkerBinding::GetConstructorObject(aCx, global)) ||

Doesn't exactly end up readable :)

@@ +1154,5 @@
>  
> +bool
> +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
> +{
> +    JS_ReportErrorNumber(aCx, js_GetErrorMessage, nullptr, JSMSG_GETTER_ONLY);

2 spaces

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can these fail?

In theory, yes, mostly due to OOM...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813004 [details] [diff] [review]
Patch

>+++ b/dom/bindings/Codegen.py
>             def addHeaderForFunc(func):
>+                if func is None:

When would it be None?

>+                if not desc.headerIsDefault:
>+                    # An explicit header file was provided, assume that we know
>+                    # what we're doing.

This requires all functions for the interface to be in that header.

Ben had some thoughts about finding the fully qualified classname for the func here, finding the descriptor with that nativeType (if any), and if so using that descriptor's headerFile.  Will that address the issue you're trying to address here?

Followup bug on doing that is ok, in the interests of getting this landed...

>+++ b/dom/bindings/test/TestBindingHeader.h
>+#include "nsGenericHTMLElement.h"

Huh.  Why is that needed?

>+++ b/dom/webidl/Worker.webidl
>+  void postMessage(any message, optional any transfer);

The second arg should really become sequence<any> at the very least, or better yet a sequence of something like Transferable...

Followup is OK, I guess, but please make it happen?

>+++ b/dom/workers/RuntimeService.cpp
>+  WorkerBinding::GetConstructorObject(aCx, aObj);
>+  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
>+  ErrorEventBinding::GetConstructorObject(aCx, aObj);
>+  MessageEventBinding::GetConstructorObject(aCx, aObj);

At least for the moment those can fail.  If they do, need to return false here.

>+++ b/dom/workers/WorkerPrivate.cpp
>+  void
>+  PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
>+               bool aDispatchResult)

virtual, and MOZ_OVERRIDE, right?

>+        ErrorEventInitInitializer init;

Drop the "Initializer" bit; we killed it off.

> WorkerPrivateParent<Derived>::PostMessage(JSContext* aCx,
>+                    aTransferable.WasPassed() ? aTransferable.Value()
>+                                              : JS::NullValue(),

Why not just give the argument "null" as a default value in the IDL?

>+WorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL,
>+  nsString scriptURL(aScriptURL);

Why do you need this extra string object?  Can you not just use aScriptURL?

>+GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
>+  if (JSVAL_IS_PRIMITIVE(aWorker)) {

  if (!aWorker.isObject()) {

Also, should aWorker be a Handle<Value>?

I'm assuming the PostDispatch bit above is reasonable.  r=me with the nits fixed and followups filed.

Comment 6

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

Attachment: Rebased patch

Rebased through the SharedWorker landing.  I've made no effort to ensure this builds or to address review comments yet.

Comment 7

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

Attachment: Interdiff needed to make things work

After this we compile and pass tests.

Comment 8

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

Attachment: Patch

Patch with bz's comments addressed

Comment 9

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

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #5)
> Comment on attachment 813004 [details] [diff] [review]
> Patch
> 
> >+++ b/dom/bindings/Codegen.py
> >             def addHeaderForFunc(func):
> >+                if func is None:
> 
> When would it be None?

I don't know, but that's what the existing code does ...

> >+                if not desc.headerIsDefault:
> >+                    # An explicit header file was provided, assume that we know
> >+                    # what we're doing.
> 
> This requires all functions for the interface to be in that header.
> 
> Ben had some thoughts about finding the fully qualified classname for the
> func here, finding the descriptor with that nativeType (if any), and if so
> using that descriptor's headerFile.  Will that address the issue you're
> trying to address here?
> 
> Followup bug on doing that is ok, in the interests of getting this landed...

I think so.  I'll file a followup.

> >+++ b/dom/bindings/test/TestBindingHeader.h
> >+#include "nsGenericHTMLElement.h"
> 
> Huh.  Why is that needed?

Because I removed the "automatically include stuff" bit above.

> >+++ b/dom/webidl/Worker.webidl
> >+  void postMessage(any message, optional any transfer);
> 
> The second arg should really become sequence<any> at the very least, or
> better yet a sequence of something like Transferable...
> 
> Followup is OK, I guess, but please make it happen?

Fixed.

> >+++ b/dom/workers/RuntimeService.cpp
> >+  WorkerBinding::GetConstructorObject(aCx, aObj);
> >+  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
> >+  ErrorEventBinding::GetConstructorObject(aCx, aObj);
> >+  MessageEventBinding::GetConstructorObject(aCx, aObj);
> 
> At least for the moment those can fail.  If they do, need to return false
> here.

Fixed.

> >+++ b/dom/workers/WorkerPrivate.cpp
> >+  void
> >+  PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
> >+               bool aDispatchResult)
> 
> virtual, and MOZ_OVERRIDE, right?

Yeah, but this file doesn't use that anywhere.  I can file a followup on adding it across the board if you like.

> >+        ErrorEventInitInitializer init;
> 
> Drop the "Initializer" bit; we killed it off.

Fixed.
 
> > WorkerPrivateParent<Derived>::PostMessage(JSContext* aCx,
> >+                    aTransferable.WasPassed() ? aTransferable.Value()
> >+                                              : JS::NullValue(),
> 
> Why not just give the argument "null" as a default value in the IDL?

This went away with the sequence change.

> >+WorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL,
> >+  nsString scriptURL(aScriptURL);
> 
> Why do you need this extra string object?  Can you not just use aScriptURL?

I don't.  Removed.

> >+GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> >+  if (JSVAL_IS_PRIMITIVE(aWorker)) {
> 
>   if (!aWorker.isObject()) {

Fixed.

> Also, should aWorker be a Handle<Value>?

I don't think it needs to be, although it probably wouldn't hurt.

> I'm assuming the PostDispatch bit above is reasonable.  r=me with the nits
> fixed and followups filed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I don't know, but that's what the existing code does ...

Oh, right.  I thought you were after that check for some reason.  It's None if not set.

> Because I removed the "automatically include stuff" bit above.

OK, please make sure to remove that in the followup?

> I can file a followup on adding it across the board if you like.

Please.

> I don't think it needs to be, although it probably wouldn't hurt.

If it's not hard, it would be vastly preferable.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 815472 [details] [diff] [review]
Rebased patch

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

This looks pretty close but there was enough stuff that I'd like to see the next version.

You should be able to remove Worker.h/Worker.cpp right? And some code in MessageEvent?

::: dom/base/nsDOMClassInfo.cpp
@@ -194,4 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::dom;
> -using mozilla::dom::workers::ResolveWorkerClasses;

There should be an #include you can lose here somewhere, right?

::: dom/bindings/Codegen.py
@@ +701,3 @@
>              if desc.interface.isExternal():
>                  continue
>              def addHeaderForFunc(func):

Yeah, maybe I forgot to file a followup on this, I can't find it now (though I could have sworn that I did...) but can you use the header from the descriptor if the native type matches?

::: dom/workers/RuntimeService.cpp
@@ +967,4 @@
>  
>  BEGIN_WORKERS_NAMESPACE
>  
> +// Entry point for XPConnect globals.

Nit: It's really for main thread non-window globals... It's not clear what "XPConnect" really means here.

@@ +1004,5 @@
>    }
>  
> +  WorkerBinding::GetConstructorObject(aCx, aObj);
> +  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
> +  ErrorEventBinding::GetConstructorObject(aCx, aObj);

I can't remember, will this handle the 'Event' binding automatically?

@@ +1007,5 @@
> +  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
> +  ErrorEventBinding::GetConstructorObject(aCx, aObj);
> +  MessageEventBinding::GetConstructorObject(aCx, aObj);
> +
> +  if (!events::InitClasses(aCx, aObj, true)) {

We need a followup to convert these.

@@ +1010,5 @@
> +
> +  if (!events::InitClasses(aCx, aObj, true)) {
> +    return false;
> +  }
> + 

Nit: extra whitespace

::: dom/workers/WorkerPrivate.cpp
@@ +81,4 @@
>  // GC will run five seconds after the last event is processed.
>  #define IDLE_GC_TIMER_DELAY_MS 5000
>  
> +#define PREF_WORKERS_PREFIX "dom.workers."

Nit: You only use this once, just add to PREF_WORKERS_ENABLED

@@ +921,5 @@
> +      // cloning into worker when array goes out of scope.
> +      nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
> +      clonedObjects.SwapElements(mClonedObjects);
> +
> +      JS::Rooted<JS::Value> messageData(aCx);

Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that you're touching. (Also need JS::HandleFoo in a few places). There are a bunch of these so please search/replace.

@@ +923,5 @@
> +      clonedObjects.SwapElements(mClonedObjects);
> +
> +      JS::Rooted<JS::Value> messageData(aCx);
> +      if (!mBuffer.read(aCx, messageData.address(),
> +                        workers::WorkerStructuredCloneCallbacks(mainRuntime))) {

I really hope the workers:: is unnecessary here?

@@ +929,5 @@
>        }
>  
> +      nsCOMPtr<nsIDOMEvent> domEvent;
> +      nsresult rv =
> +        nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr,

Blegh, just make a new nsDOMMessageEvent like DispatchMessageEventToMessagePort() does. No need for this extra indirection.

@@ +932,5 @@
> +      nsresult rv =
> +        nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr,
> +                                       NS_LITERAL_STRING("messageevent"),
> +                                       getter_AddRefs(domEvent));
> +      NS_ENSURE_SUCCESS(rv, false);

This (and your other NS_ENSURE_SUCCESS below) will trigger an uncatchable exception. You need to |Throw(aCx, rv)| before returning false from this method.

@@ +944,5 @@
> +                                     EmptyString(),
> +                                     nullptr);
> +      NS_ENSURE_SUCCESS(rv, false);
> +
> +      message->SetTrusted(aWorkerPrivate->IsChromeWorker());

This should always be true. Trusted events just mean "generated by the browser", not by page script.

@@ +951,5 @@
> +      aWorkerPrivate->DispatchDOMEvent(nullptr, message, nullptr, &dummy);
> +      return true;
> +    }
> +
> +    NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx),

Nit: Since you're touching this you might as well make this MOZ_ASSERT

@@ +955,5 @@
> +    NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx),
> +                 "Badness!");
> +    if (mToMessagePort) {
> +      WorkerMessagePort* port =
> +        aWorkerPrivate->GetMessagePort(mMessagePortSerial);

Crap, I know this isn't your fault, but can you make that a nsRefPtr? It's bad to call out without ensuring a ref.

@@ +1148,4 @@
>      WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult);
>    }
>  
> +  // NB: aWorkerPrivate is the worker thread we're on (or the main thread, if

Nit: 'NB' or 'Note' or 'Important!' and other such stuff in a comment is silly. You're already calling it out and if it wasn't important I would tell you not to add it ;)

@@ +1191,3 @@
>  
> +        nsRefPtr<ErrorEvent> event =
> +          ErrorEvent::Constructor(aTarget, NS_LITERAL_STRING("error"), init);

This could fail, right?

@@ +2082,4 @@
>    MOZ_ASSERT_IF(!aIsSharedWorker, aObject && aSharedWorkerName.IsEmpty());
>  
>    if (aLoadInfo.mWindow) {
> +    MOZ_ASSERT(NS_IsMainThread());

Nit: AssertIsOnMainThread()

@@ +2126,5 @@
> +
> +template <class Derived>
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>,
> +                                                  nsDOMEventTargetHelper)
> +  // Nothing else to traverse

Mention that LoadInfo stuff here too?

@@ +2466,5 @@
> +  ClearWrapper();
> +
> +  // Ensure that we're held alive across the TerminatePrivate call, and then
> +  // release the reference our wrapper held to us.
> +  nsRefPtr<WorkerPrivateParent<Derived> > kungFuDeathGrip = dont_AddRef(this);

Nit: >>, here and in a few other places

@@ +2545,2 @@
>      }
> +    NS_RELEASE_THIS();

Wait, you can't call this and then touch mRooted, can you? Are you guaranteed to never be releasing the last ref here? I think you need something a little trickier here.

@@ +2614,4 @@
>      }
>    }
>  
> +  JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());

Nit: Hm, undefined should be the default right?

@@ +2618,5 @@
> +  if (aTransferable.WasPassed()) {
> +    const Sequence<JS::Value>& realTransferable = aTransferable.Value();
> +    JSObject* array =
> +      JS_NewArrayObject(aCx, realTransferable.Length(),
> +                        const_cast<jsval*>(realTransferable.Elements()));

JS::RootedObject for the array, also (nit) s/jsval/JS::Value/

@@ +2631,5 @@
>  
>    JSAutoStructuredCloneBuffer buffer;
> +  if (!buffer.write(aCx, aMessage, transferable.WasPassed(), callbacks,
> +                    &clonedObjects)) {
> +    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);

Hrm. Does our code deal with this properly? If there's a pending exception set on aCx after write() returns then this Throw() call won't overwrite it, will it?

@@ +3287,3 @@
>  {
> +  return WorkerPrivate::Constructor(aGlobal, aScriptURL, false, false,
> +                                    EmptyString(), aRv);

Can this not be inlined?

@@ +3294,5 @@
> +WorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> +{
> +  // If we're already on a worker workers are clearly enabled.
> +  if (!NS_IsMainThread()) {
> +    return true;

You could assert GetWorkerPrivateFromContext() here.

@@ +3298,5 @@
> +    return true;
> +  }
> +
> +  // If our caller is chrome, workers are always available.
> +  if (nsContentUtils::IsCallerChrome()) {

Actually, this might be wrong, chrome calling a content window's Worker constructor should fail if the pref isn't set, shouldn't it?

@@ +3312,5 @@
> +ChromeWorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL,
> +                                 mozilla::ErrorResult& aRv)
> +{
> +  return WorkerPrivate::Constructor(aGlobal, aScriptURL, true, false,
> +                                    EmptyString(), aRv).downcast<ChromeWorkerPrivate>();

This seems inline-able too

@@ +3320,5 @@
> +bool
> +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> +{
> +  // Chrome is always allowed to use workers, and content is never allowed to
> +  // use ChromeWorker, so all we have to check 

Nit: incomplete comment

@@ +3321,5 @@
> +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> +{
> +  // Chrome is always allowed to use workers, and content is never allowed to
> +  // use ChromeWorker, so all we have to check 
> +  return nsContentUtils::ThreadsafeIsCallerChrome();

Same concern about chrome code calling this on a content window.

@@ +3332,5 @@
> +                           const nsAString& aSharedWorkerName,
> +                           LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv)
> +{
> +  WorkerPrivate* parent = NS_IsMainThread() ? nullptr
> +                                            : GetCurrentThreadWorkerPrivate();

Nit: pattern usually is all on one line or:

  foo = cond ?
        bar :
        baz;

@@ +3367,5 @@
> +  if (!parent) {
> +    runtimeService = RuntimeService::GetOrCreateService();
> +    if (!runtimeService) {
> +      JS_ReportError(cx, "Failed to create runtime service!");
> +      aRv.Throw(NS_ERROR_FAILURE);

Same concern above, which error will JS actually see?

@@ +3372,5 @@
> +      return nullptr;
> +    }
> +  }
> +  else {
> +    runtimeService = RuntimeService::GetService();

You should MOZ_ASSERT |runtimeService| after this.

@@ +3390,5 @@
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  // Worker now also owned by its thread.

Nit: 'also' doesn't make sense here like it did before. This should be more verbose maybe.

@@ +5312,5 @@
>  
>  BEGIN_WORKERS_NAMESPACE
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)

Something is funny, I don't see where in this patch you're moving this from. Splinter bug?

In any case, JS::Value

@@ +5314,5 @@
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> +{
> +  if (JSVAL_IS_PRIMITIVE(aWorker)) {

Nit: aWorker.isPrimitive()

@@ +5321,5 @@
> +
> +  WorkerPrivate* w = nullptr;
> +  UNWRAP_OBJECT(Worker, aCx, &aWorker.toObject(), w);
> +  if (!w) {
> +    return nullptr;

This should probably be asserted, only C++ can call this.

::: dom/workers/WorkerPrivate.h
@@ +365,4 @@
>  
>  public:
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,

Nit: return type on its own line

@@ +365,5 @@
>  
>  public:
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

JS::HandleObject

@@ +438,2 @@
>    PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +              const Optional<Sequence<JS::Value> >& aTransferable,

Nit: >> is fine now, in all the places you're touching.

@@ +797,1 @@
>    ~WorkerPrivate();

Hm, can this be protected now?

@@ +1027,5 @@
>    GetMessagePort(uint64_t aMessagePortSerial);
>  
> +protected:
> +
> +  static already_AddRefed<WorkerPrivate>

Nit: No need for the newline above.

@@ +1118,5 @@
>  
> +// This class is only used to trick the DOM bindings.  We never create
> +// instances of it, and static_casting to it is fine since it doesn't add
> +// anything to WorkerPrivate.
> +class ChromeWorkerPrivate : public WorkerPrivate {

Nit: { on its own line

@@ +1128,5 @@
> +  static bool
> +  WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */);
> +
> +private:
> +  ChromeWorkerPrivate() MOZ_DELETE;

Add (er, delete) the copy constructor too?

::: dom/workers/WorkerScope.cpp
@@ +1465,5 @@
>    }
>  
>    // Init other paris-bindings.
> +  if (!(!worker->IsChromeWorker() ||
> +        ChromeWorkerBinding::GetConstructorObject(aCx, global)) ||

Eww! Move this above to the block with the other chrome worker stuff.

@@ +1491,5 @@
>    return global;
>  }
>  
> +bool
> +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp)

I don't understand why this is here... Another Splinter bug?

Comment 12

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

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11)
> ::: dom/workers/WorkerPrivate.cpp
> @@ +921,5 @@
> > +      // cloning into worker when array goes out of scope.
> > +      nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
> > +      clonedObjects.SwapElements(mClonedObjects);
> > +
> > +      JS::Rooted<JS::Value> messageData(aCx);
> 
> Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that
> you're touching. (Also need JS::HandleFoo in a few places). There are a
> bunch of these so please search/replace.

Not in dom/.

Comment 13

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

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11)
> Comment on attachment 815472 [details] [diff] [review]
> Rebased patch
> 
> Review of attachment 815472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty close but there was enough stuff that I'd like to see the
> next version.
> 
> You should be able to remove Worker.h/Worker.cpp right? And some code in
> MessageEvent?

Yes, git rm Worker.h/cpp was just missing from this.  idk about MessageEvent, I would prefer to just convert the global (which I have another patch for) and kill the worker events entirely.

> ::: dom/base/nsDOMClassInfo.cpp
> @@ -194,4 @@
> >  
> >  using namespace mozilla;
> >  using namespace mozilla::dom;
> > -using mozilla::dom::workers::ResolveWorkerClasses;
> 
> There should be an #include you can lose here somewhere, right?

Yeah.

> ::: dom/bindings/Codegen.py
> @@ +701,3 @@
> >              if desc.interface.isExternal():
> >                  continue
> >              def addHeaderForFunc(func):
> 
> Yeah, maybe I forgot to file a followup on this, I can't find it now (though
> I could have sworn that I did...) but can you use the header from the
> descriptor if the native type matches?

bz talked about it.  We can do it in a followup.

> ::: dom/workers/RuntimeService.cpp
> @@ +967,4 @@
> >  
> >  BEGIN_WORKERS_NAMESPACE
> >  
> > +// Entry point for XPConnect globals.
> 
> Nit: It's really for main thread non-window globals... It's not clear what
> "XPConnect" really means here.

Ok.

> @@ +1004,5 @@
> >    }
> >  
> > +  WorkerBinding::GetConstructorObject(aCx, aObj);
> > +  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
> > +  ErrorEventBinding::GetConstructorObject(aCx, aObj);
> 
> I can't remember, will this handle the 'Event' binding automatically?

Yes.

> @@ +1007,5 @@
> > +  ChromeWorkerBinding::GetConstructorObject(aCx, aObj);
> > +  ErrorEventBinding::GetConstructorObject(aCx, aObj);
> > +  MessageEventBinding::GetConstructorObject(aCx, aObj);
> > +
> > +  if (!events::InitClasses(aCx, aObj, true)) {
> 
> We need a followup to convert these.

events?  We'll just get rid of them entirely soon.

> @@ +1010,5 @@
> > +
> > +  if (!events::InitClasses(aCx, aObj, true)) {
> > +    return false;
> > +  }
> > + 
> 
> Nit: extra whitespace
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +81,4 @@
> >  // GC will run five seconds after the last event is processed.
> >  #define IDLE_GC_TIMER_DELAY_MS 5000
> >  
> > +#define PREF_WORKERS_PREFIX "dom.workers."
> 
> Nit: You only use this once, just add to PREF_WORKERS_ENABLED

Done.

> @@ +921,5 @@
> > +      // cloning into worker when array goes out of scope.
> > +      nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
> > +      clonedObjects.SwapElements(mClonedObjects);
> > +
> > +      JS::Rooted<JS::Value> messageData(aCx);
> 
> Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that
> you're touching. (Also need JS::HandleFoo in a few places). There are a
> bunch of these so please search/replace.

dom/ doesn't do this.  Please file another bug on doing a global search/replace.

> @@ +923,5 @@
> > +      clonedObjects.SwapElements(mClonedObjects);
> > +
> > +      JS::Rooted<JS::Value> messageData(aCx);
> > +      if (!mBuffer.read(aCx, messageData.address(),
> > +                        workers::WorkerStructuredCloneCallbacks(mainRuntime))) {
> 
> I really hope the workers:: is unnecessary here?

It's needed to disambiguate http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/workers/WorkerPrivate.cpp#l220 and http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/workers/WorkerPrivate.h#l1121.

> @@ +929,5 @@
> >        }
> >  
> > +      nsCOMPtr<nsIDOMEvent> domEvent;
> > +      nsresult rv =
> > +        nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr,
> 
> Blegh, just make a new nsDOMMessageEvent like
> DispatchMessageEventToMessagePort() does. No need for this extra indirection.

Ok.

> @@ +932,5 @@
> > +      nsresult rv =
> > +        nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr,
> > +                                       NS_LITERAL_STRING("messageevent"),
> > +                                       getter_AddRefs(domEvent));
> > +      NS_ENSURE_SUCCESS(rv, false);
> 
> This (and your other NS_ENSURE_SUCCESS below) will trigger an uncatchable
> exception. You need to |Throw(aCx, rv)| before returning false from this
> method.

Ok.

> @@ +944,5 @@
> > +                                     EmptyString(),
> > +                                     nullptr);
> > +      NS_ENSURE_SUCCESS(rv, false);
> > +
> > +      message->SetTrusted(aWorkerPrivate->IsChromeWorker());
> 
> This should always be true. Trusted events just mean "generated by the
> browser", not by page script.

Uh, yeah, idk wtf I was doing there.

> @@ +951,5 @@
> > +      aWorkerPrivate->DispatchDOMEvent(nullptr, message, nullptr, &dummy);
> > +      return true;
> > +    }
> > +
> > +    NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx),
> 
> Nit: Since you're touching this you might as well make this MOZ_ASSERT

Ok.

> @@ +955,5 @@
> > +    NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx),
> > +                 "Badness!");
> > +    if (mToMessagePort) {
> > +      WorkerMessagePort* port =
> > +        aWorkerPrivate->GetMessagePort(mMessagePortSerial);
> 
> Crap, I know this isn't your fault, but can you make that a nsRefPtr? It's
> bad to call out without ensuring a ref.

Fixed.

> @@ +1148,4 @@
> >      WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult);
> >    }
> >  
> > +  // NB: aWorkerPrivate is the worker thread we're on (or the main thread, if
> 
> Nit: 'NB' or 'Note' or 'Important!' and other such stuff in a comment is
> silly. You're already calling it out and if it wasn't important I would tell
> you not to add it ;)

Ok.

> @@ +1191,3 @@
> >  
> > +        nsRefPtr<ErrorEvent> event =
> > +          ErrorEvent::Constructor(aTarget, NS_LITERAL_STRING("error"), init);
> 
> This could fail, right?

No.  Generated event ctors are infallible.

> @@ +2082,4 @@
> >    MOZ_ASSERT_IF(!aIsSharedWorker, aObject && aSharedWorkerName.IsEmpty());
> >  
> >    if (aLoadInfo.mWindow) {
> > +    MOZ_ASSERT(NS_IsMainThread());
> 
> Nit: AssertIsOnMainThread()

Done.

> @@ +2126,5 @@
> > +
> > +template <class Derived>
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>,
> > +                                                  nsDOMEventTargetHelper)
> > +  // Nothing else to traverse
> 
> Mention that LoadInfo stuff here too?

Ok.

> @@ +2466,5 @@
> > +  ClearWrapper();
> > +
> > +  // Ensure that we're held alive across the TerminatePrivate call, and then
> > +  // release the reference our wrapper held to us.
> > +  nsRefPtr<WorkerPrivateParent<Derived> > kungFuDeathGrip = dont_AddRef(this);
> 
> Nit: >>, here and in a few other places

That compiles now?

> @@ +2545,2 @@
> >      }
> > +    NS_RELEASE_THIS();
> 
> Wait, you can't call this and then touch mRooted, can you? Are you
> guaranteed to never be releasing the last ref here? I think you need
> something a little trickier here.

The caller should be holding a reference (if not literally, then ensuring that something else is for the duration of the call).  Truth be told I'm not entirely convinced the addref/release are necessary here.

> @@ +2614,4 @@
> >      }
> >    }
> >  
> > +  JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());
> 
> Nit: Hm, undefined should be the default right?

Maybe.  I copied this from the existing code.

> @@ +2618,5 @@
> > +  if (aTransferable.WasPassed()) {
> > +    const Sequence<JS::Value>& realTransferable = aTransferable.Value();
> > +    JSObject* array =
> > +      JS_NewArrayObject(aCx, realTransferable.Length(),
> > +                        const_cast<jsval*>(realTransferable.Elements()));
> 
> JS::RootedObject for the array, also (nit) s/jsval/JS::Value/

This is code you wrote last week you're complaining about ...

> @@ +2631,5 @@
> >  
> >    JSAutoStructuredCloneBuffer buffer;
> > +  if (!buffer.write(aCx, aMessage, transferable.WasPassed(), callbacks,
> > +                    &clonedObjects)) {
> > +    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
> 
> Hrm. Does our code deal with this properly? If there's a pending exception
> set on aCx after write() returns then this Throw() call won't overwrite it,
> will it?

Yes.  http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/bindings/Exceptions.cpp#l106

> @@ +3287,3 @@
> >  {
> > +  return WorkerPrivate::Constructor(aGlobal, aScriptURL, false, false,
> > +                                    EmptyString(), aRv);
> 
> Can this not be inlined?

*shrug*

> @@ +3294,5 @@
> > +WorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> > +{
> > +  // If we're already on a worker workers are clearly enabled.
> > +  if (!NS_IsMainThread()) {
> > +    return true;
> 
> You could assert GetWorkerPrivateFromContext() here.

assert that we're on a worker that is?

> @@ +3298,5 @@
> > +    return true;
> > +  }
> > +
> > +  // If our caller is chrome, workers are always available.
> > +  if (nsContentUtils::IsCallerChrome()) {
> 
> Actually, this might be wrong, chrome calling a content window's Worker
> constructor should fail if the pref isn't set, shouldn't it?

This is what the existing code in ResolveWorkerClasses does.  If you want it to do something different please file a followup.

> @@ +3312,5 @@
> > +ChromeWorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL,
> > +                                 mozilla::ErrorResult& aRv)
> > +{
> > +  return WorkerPrivate::Constructor(aGlobal, aScriptURL, true, false,
> > +                                    EmptyString(), aRv).downcast<ChromeWorkerPrivate>();
> 
> This seems inline-able too

Do we really care?

> @@ +3320,5 @@
> > +bool
> > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> > +{
> > +  // Chrome is always allowed to use workers, and content is never allowed to
> > +  // use ChromeWorker, so all we have to check 
> 
> Nit: incomplete comment

Fixed.

> @@ +3321,5 @@
> > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */)
> > +{
> > +  // Chrome is always allowed to use workers, and content is never allowed to
> > +  // use ChromeWorker, so all we have to check 
> > +  return nsContentUtils::ThreadsafeIsCallerChrome();
> 
> Same concern about chrome code calling this on a content window.

Same response ;-)

> @@ +3332,5 @@
> > +                           const nsAString& aSharedWorkerName,
> > +                           LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv)
> > +{
> > +  WorkerPrivate* parent = NS_IsMainThread() ? nullptr
> > +                                            : GetCurrentThreadWorkerPrivate();
> 
> Nit: pattern usually is all on one line or:
> 
>   foo = cond ?
>         bar :
>         baz;

Ok.

> @@ +3367,5 @@
> > +  if (!parent) {
> > +    runtimeService = RuntimeService::GetOrCreateService();
> > +    if (!runtimeService) {
> > +      JS_ReportError(cx, "Failed to create runtime service!");
> > +      aRv.Throw(NS_ERROR_FAILURE);
> 
> Same concern above, which error will JS actually see?
> 
> @@ +3372,5 @@
> > +      return nullptr;
> > +    }
> > +  }
> > +  else {
> > +    runtimeService = RuntimeService::GetService();
> 
> You should MOZ_ASSERT |runtimeService| after this.

Done.

> @@ +3390,5 @@
> > +    aRv.Throw(NS_ERROR_UNEXPECTED);
> > +    return nullptr;
> > +  }
> > +
> > +  // Worker now also owned by its thread.
> 
> Nit: 'also' doesn't make sense here like it did before. This should be more
> verbose maybe.

Ok.

> @@ +5312,5 @@
> >  
> >  BEGIN_WORKERS_NAMESPACE
> >  
> > +WorkerCrossThreadDispatcher*
> > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> 
> Something is funny, I don't see where in this patch you're moving this from.
> Splinter bug?

This comes from Worker.cpp.

> In any case, JS::Value

ok.

> @@ +5314,5 @@
> >  
> > +WorkerCrossThreadDispatcher*
> > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> > +{
> > +  if (JSVAL_IS_PRIMITIVE(aWorker)) {
> 
> Nit: aWorker.isPrimitive()

bz told me !isObject() :-P

> @@ +5321,5 @@
> > +
> > +  WorkerPrivate* w = nullptr;
> > +  UNWRAP_OBJECT(Worker, aCx, &aWorker.toObject(), w);
> > +  if (!w) {
> > +    return nullptr;
> 
> This should probably be asserted, only C++ can call this.

Ok.

> ::: dom/workers/WorkerPrivate.h
> @@ +365,4 @@
> >  
> >  public:
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx,
> 
> Nit: return type on its own line

Ok.

> @@ +365,5 @@
> >  
> >  public:
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx,
> > +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> 
> JS::HandleObject
> 
> @@ +438,2 @@
> >    PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> > +              const Optional<Sequence<JS::Value> >& aTransferable,
> 
> Nit: >> is fine now, in all the places you're touching.

Are you sure?

> @@ +797,1 @@
> >    ~WorkerPrivate();
> 
> Hm, can this be protected now?

Yes.

> @@ +1027,5 @@
> >    GetMessagePort(uint64_t aMessagePortSerial);
> >  
> > +protected:
> > +
> > +  static already_AddRefed<WorkerPrivate>
> 
> Nit: No need for the newline above.

Ok.

> @@ +1118,5 @@
> >  
> > +// This class is only used to trick the DOM bindings.  We never create
> > +// instances of it, and static_casting to it is fine since it doesn't add
> > +// anything to WorkerPrivate.
> > +class ChromeWorkerPrivate : public WorkerPrivate {
> 
> Nit: { on its own line

Ok.

> @@ +1128,5 @@
> > +  static bool
> > +  WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */);
> > +
> > +private:
> > +  ChromeWorkerPrivate() MOZ_DELETE;
> 
> Add (er, delete) the copy constructor too?

Ok.  I threw in operator= too.

> ::: dom/workers/WorkerScope.cpp
> @@ +1465,5 @@
> >    }
> >  
> >    // Init other paris-bindings.
> > +  if (!(!worker->IsChromeWorker() ||
> > +        ChromeWorkerBinding::GetConstructorObject(aCx, global)) ||
> 
> Eww! Move this above to the block with the other chrome worker stuff.

Ok.

> @@ +1491,5 @@
> >    return global;
> >  }
> >  
> > +bool
> > +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
> 
> I don't understand why this is here... Another Splinter bug?

More stuff I moved out of Worker.cpp

Comment 14

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

Attachment: Interdiff

Comment 15

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

Attachment: Full patch

Comment 16

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

Comment on attachment 815799 [details] [diff] [review]
Full patch

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

::: dom/bindings/Bindings.conf
@@ +31,5 @@
>  #                   true for workers, false otherwise).
>  #   * customFinalize - The native class will use a custom finalize hook
>  #                      (defaults to true for workers, false otherwise).
> +#   * customWrapperManagement - The native class will be responsible for
> +#                               preserving its own wrapper (no AddProperty)

This ) may be in the wrong position.

::: dom/workers/RuntimeService.cpp
@@ +1012,2 @@
>  
> +  if (!events::InitClasses(aCx, aObj, true)) {

Hrm. Looks like this will define WorkerEvent and friends, but not when you're actually resolving those? (Because you removed them from gStringChars.)

@@ +2000,4 @@
>    rv = loadInfo.mResolvedScriptURI->GetSpec(scriptSpec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsRefPtr<WorkerPrivate> workerPrivate = nullptr;

Don't need the = nullptr anymore

@@ +2020,5 @@
> +    ErrorResult rv;
> +    workerPrivate =
> +      WorkerPrivate::Constructor(aGlobal, aScriptURL, false, true, aName,
> +                                 &loadInfo, rv);
> +    NS_ENSURE_TRUE(workerPrivate, NS_ERROR_FAILURE);

Not checking rv?

::: dom/workers/SharedWorker.cpp
@@ +35,4 @@
>    mSuspended(false)
>  {
>    AssertIsOnMainThread();
> +  MOZ_ASSERT(mWorkerPrivate);

Why these changes?

@@ +78,4 @@
>    }
>  
>    nsRefPtr<SharedWorker> sharedWorker;
> +  nsresult rv = rts->CreateSharedWorker(aGlobal, aScriptURL, name,

Followup on removing the |'implicitJSContext': [ 'constructor' ],| bit from Bindings.conf, please.

::: dom/workers/Worker.cpp
@@ -1,1 @@
> -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

Woo!

::: dom/workers/WorkerPrivate.cpp
@@ +923,5 @@
> +
> +      JS::Rooted<JS::Value> messageData(aCx);
> +      if (!mBuffer.read(aCx, messageData.address(),
> +                        workers::WorkerStructuredCloneCallbacks(!aWorkerPrivate->GetParent()))) {
> +        xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR);

dom::Throw?

@@ +938,5 @@
> +                                EmptyString(),
> +                                EmptyString(),
> +                                nullptr);
> +      if (NS_FAILED(rv)) {
> +        xpc::Throw(aCx, rv);

Ditto

@@ +1181,5 @@
>      // they show up in the error console.
>      if (!JSREPORT_IS_WARNING(aFlags)) {
>        // First fire an ErrorEvent at the worker.
> +      if (aTarget) {
> +        ErrorEventInit init;

Much nicer :)

@@ +2541,3 @@
>      }
> +  }
> +  else {

} else {

@@ +2585,3 @@
>                                              bool aToMessagePort,
> +                                            uint64_t aMessagePortSerial,
> +                                            mozilla::ErrorResult& aRv)

Drop the mozilla::?

@@ +2619,5 @@
> +  if (aTransferable.WasPassed()) {
> +    const Sequence<JS::Value>& realTransferable = aTransferable.Value();
> +    JSObject* array =
> +      JS_NewArrayObject(aCx, realTransferable.Length(),
> +                        const_cast<JS::Value*>(realTransferable.Elements()));

Followup to make JS_NewArrayObject take a const pointer?

@@ +3281,5 @@
>  // static
>  already_AddRefed<WorkerPrivate>
> +WorkerPrivate::Constructor(const GlobalObject& aGlobal,
> +                           const nsAString& aScriptURL,
> +                           mozilla::ErrorResult& aRv)

Should be able to drop this mozilla:: too

@@ +3309,5 @@
> +// static
> +already_AddRefed<ChromeWorkerPrivate>
> +ChromeWorkerPrivate::Constructor(const GlobalObject& aGlobal,
> +                                 const nsAString& aScriptURL,
> +                                 mozilla::ErrorResult& aRv)

Ditto

@@ +3374,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }
> +  }
> +  else {

} else {

@@ +5312,5 @@
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, JS::Value aWorker)
> +{
> +  if (aWorker.isPrimitive()) {

!isObject() is nicer, as you do toObject() below

::: dom/workers/WorkerPrivate.h
@@ +303,5 @@
>    WorkerPrivate* mParent;
>    nsString mScriptURL;
>    nsString mSharedWorkerName;
>    LocationInfo mLocationInfo;
> +  // NB: The lifetime of these objects within LoadInfo is managed explicitly;

Another NB

@@ +798,5 @@
>  
> +public:
> +  static already_AddRefed<WorkerPrivate>
> +  Constructor(const GlobalObject& aGlobal, const nsAString& aScriptURL,
> +              ErrorResult& rv);

aRv

@@ +804,5 @@
>    static already_AddRefed<WorkerPrivate>
> +  Constructor(const GlobalObject& aGlobal, const nsAString& aScriptURL,
> +              bool aIsChromeWorker, bool aIsSharedWorker,
> +              const nsAString& aSharedWorkerName,
> +              LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv);

Drop mozilla::

@@ +1028,4 @@
>    GetMessagePort(uint64_t aMessagePortSerial);
>  
>  private:
> +  WorkerPrivate(JSContext* aCx,  WorkerPrivate* aParent,

Double space after |aCx,|

::: dom/workers/WorkerScope.cpp
@@ +1492,5 @@
>  
> +bool
> +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
> +{
> +    JS_ReportErrorNumber(aCx, js_GetErrorMessage, nullptr, JSMSG_GETTER_ONLY);

Two-space indentation

Comment 17

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

I made all of Ms2ger's suggested changes except:

- Changing the else cuddling: half cuddling is consistent with the rest of the directory and other bent code.
- Changing xpc::Throw to dom::Throw: using xpc::Throw is consistent with the rest of this file, and since that may change behavior I'd prefer to fix it in a followup.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 815797 [details] [diff] [review]
Interdiff

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

::: dom/workers/WorkerPrivate.cpp
@@ +2126,5 @@
>  template <class Derived>
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>,
>                                                    nsDOMEventTargetHelper)
>    // Nothing else to traverse
> +  // The various strong references in LoadInfo are managed manually andcannot

Nit: s/andcannot/and cannot/

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #13)
> dom/ doesn't do this.  Please file another bug on doing a global
> search/replace.

Really? Crap. I coulda sworn the newsgroup decision was to use the typedefs from now on in Gecko.

> That compiles now?

Yep, I added a few with SharedWorker.

> The caller should be holding a reference (if not literally, then ensuring
> that something else is for the duration of the call).

Maybe, but that wasn't the case before (since GC would have to happen after this call finished). Have you checked the code here or are you just philosophizing? 

> This is code you wrote last week you're complaining about ...

Ha! Technically it was "many months ago" and I blame my reviewer ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I coulda sworn the newsgroup decision was to use the typedefs from now on in Gecko.

The only decision so far was about the JS engine...

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/91495d3a91c8

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bb002bc13a - one of those two broke osfile/tests/xpcshell/test_exception.js rather thoroughly, https://tbpl.mozilla.org/php/getParsedLog.php?id=29068111&tree=Mozilla-Inbound.

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db9190b3ce60

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent Android Armv6 mochitest-8 crashes caused by either this or bug 919885.
https://hg.mozilla.org/mozilla-central/rev/30afbcdcec4d

https://tbpl.mozilla.org/php/getParsedLog.php?id=29092370&tree=Mozilla-Central

Comment 25

[Ryan VanderMeulen [:RyanVM]]

More stacks:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29091987&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=29082049&tree=Mozilla-Inbound

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Retriggers are green post-backout.

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c5b68ae2e9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22c5b68ae2e9