1250572 - Force a parent object in MessagePort/Channel and in StructuredCloneHolder

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: mpInWorker2.patch

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The reason for this bug is?

SendRunnable::MainThreadRun() could sure use some global explicitly.
That code should switch to use AutoJSAPI. It is not clear to me which global that code uses otherwise.
(I guess or hope AutoSafeJSContext uses junk scope, but I don't know that.)

Comment 2

[Andrea Marchesini [:baku]]

This is the last step before replacing PostMessage() with MessagePort::PostMessage().
This patch is green on try. Actually I would like to replace nsISupport in StructureCloneHolder with nsIGlobalObject, but that can be a follow up.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8722575 [details] [diff] [review]
mpInWorker2.patch


>   bool
>   DispatchDOMEvent(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
>                    DOMEventTargetHelper* aTarget, bool aIsMainThread)
>   {
>-    nsCOMPtr<nsPIDOMWindowInner> parent;
>-    if (aIsMainThread) {
>-      parent = do_QueryInterface(aTarget->GetParentObject());
>+    JS::Rooted<JSObject*> globalObject(aCx, JS::CurrentGlobalOrNull(aCx));
>+    if (NS_WARN_IF(!globalObject)) {
>+      return false;
>+    }
>+
>+    nsCOMPtr<nsIGlobalObject> parent = xpc::NativeGlobal(globalObject);
>+    if (NS_WARN_IF(!parent)) {
>+      return false;
>     }
I don't understand this change. Why can't we use the global from aTarget?
(any manual JS API usage is a bug in my book so better to avoid it when possible ;) )



>@@ -6346,29 +6351,35 @@ WorkerPrivate::ConnectMessagePort(JSCont
> {
>   AssertIsOnWorkerThread();
> 
>   WorkerGlobalScope* globalScope = GlobalScope();
> 
>   JS::Rooted<JSObject*> jsGlobal(aCx, globalScope->GetWrapper());
>   MOZ_ASSERT(jsGlobal);
> 
>+  GlobalObject globalObject(aCx, jsGlobal);
>+  if (globalObject.Failed()) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIGlobalObject> global =
>+    do_QueryInterface(globalObject.GetAsSupports());
>+  if (!global) {
>+    return false;
>+  }
Why you need to do all this when globalScope already _is_a_ global object?

>@@ -1324,17 +1325,22 @@ EventRunnable::WorkerRun(JSContext* aCx,
>     state->mResponseResult = mResponseResult;
> 
>     if (NS_SUCCEEDED(mResponseResult)) {
>       if (HasData()) {
>         MOZ_ASSERT(mResponse.isUndefined());
> 
>         ErrorResult rv;
>         JS::Rooted<JS::Value> response(aCx);
>-        Read(nullptr, aCx, &response, rv);
>+
>+        GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
>+        nsCOMPtr<nsIGlobalObject> global =
>+          do_QueryInterface(globalObj.GetAsSupports());
>+
Why all this when aWorkerPrivate->GlobalScope() is already the global object?


>@@ -1501,18 +1507,28 @@ SendRunnable::MainThreadRun()
>     AutoSafeJSContext cx;
>     JSAutoRequest ar(cx);
ok, this could should use AutoJSAPI, but looks like 
AutoSafeJSContext ends up using junk scope, so fine.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: mpInWorker2.patch

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c42ff6847631

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8723059 [details] [diff] [review]
mpInWorker2.patch

>+++ b/dom/workers/WorkerPrivate.cpp
>@@ -636,20 +636,18 @@ public:
>   bool
>   DispatchDOMEvent(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
>                    DOMEventTargetHelper* aTarget, bool aIsMainThread)
>   {
>-    nsCOMPtr<nsPIDOMWindowInner> parent;
>-    if (aIsMainThread) {
>-      parent = do_QueryInterface(aTarget->GetParentObject());
>-    }
>+    nsCOMPtr<nsIGlobalObject> parent = do_QueryInterface(aTarget->GetParentObject());
>+    MOZ_ASSERT(parent);

This new assertion fails for me, in my local inbound build, during startup. (So, I can no longer run my local firefox build, as of this commit.)

My mozconfig is pretty simple:
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj
mk_add_options AUTOCLOBBER=1
ac_add_options --enable-debug --disable-optimize
ac_add_options --enable-warnings-as-errors
ac_add_options --enable-profiling
and I'm on 64-bit Linux, building with clang.

In my case, aTarget->mParentObject is nullptr.  Happy to provide more info as-needed.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: backtrace of startup abort

Comment 8

[Daniel Holbert [:dholbert]]

(My build is based off of 87064fa7de53, which is the child of this bug's commit from comment 5.)

Comment 9

[Daniel Holbert [:dholbert]]

This abort is showing up on TreeHerder as well, as Linux64 debug reds on e.g. this push:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=87064fa7de53

So, I backed this out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f601e5ce85

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/124b69064618

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/124b69064618