Closed Bug 1250572 Opened 4 years ago Closed 4 years ago

Force a parent object in MessagePort/Channel and in StructuredCloneHolder

Categories

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

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 1 obsolete file)

Attached patch mpInWorker2.patch (obsolete) — Splinter Review
No description provided.
Attachment #8722575 - Flags: review?(bugs)
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.)
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 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.
Attachment #8722575 - Flags: review?(bugs) → review-
Attachment #8722575 - Attachment is obsolete: true
Attachment #8723059 - Flags: review?(bugs)
Attachment #8723059 - Flags: review?(bugs) → review+
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.
(My build is based off of 87064fa7de53, which is the child of this bug's commit from comment 5.)
Whiteboard: btpp-active
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/124b69064618
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1253199
Depends on: 1286904
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.