Closed Bug 1638910 Opened 5 years ago Closed 5 years ago

Crash from JSWindowActorProtocol::RegisterListenersFor against a null window

Categories

(Core :: DOM: Navigation, defect)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: ochameau, Assigned: nika)

References

Details

Attachments

(1 file, 1 obsolete file)

JSWindowActorProtocol::RegisterListenersFor:
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/dom/ipc/JSWindowActorProtocol.cpp#252
is being called with aTarget referencing a WindowRoot which is having a null mWindow.
This later crashes in WindowRoot::GetOwnerGlobal:
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/dom/base/nsWindowRoot.cpp#115-120
because mWindow is null.

I worked around that with following patch:

--- a/dom/base/nsWindowRoot.cpp
+++ b/dom/base/nsWindowRoot.cpp
@@ -108,16 +108,20 @@ nsresult nsWindowRoot::PostHandleEvent(E
   return NS_OK;
 }
 
 nsPIDOMWindowOuter* nsWindowRoot::GetOwnerGlobalForBindingsInternal() {
   return mWindow;
 }
 
 nsIGlobalObject* nsWindowRoot::GetOwnerGlobal() const {
+  if (!mWindow) {
+    printf("GetOwnerGlobal null mWindow, would have crashed?\n");
+    return nullptr;
+  }
   nsCOMPtr<nsIGlobalObject> global =
       do_QueryInterface(mWindow->GetCurrentInnerWindow());
   // We're still holding a ref to it, so returning the raw pointer is ok...
   return global;
 }
 
 nsPIDOMWindowOuter* nsWindowRoot::GetWindow() { return mWindow; }

I don't know if that's a relevant fix or not?
Should we instead bail out early from JSWindowActorProtocol::RegisterListenersFor if the aTarget is referencing a destroyed object?
Or one layer upper, from JSActorService::LoadJSActorInfos?
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/dom/ipc/JSActorService.cpp#135-138
Or is there an issue with JSActorService::UnregisterChromeEventTarget not being called for all destroyed windows? Or called a bit too late?

Unfortunately, I do not reproduce this crash with a simple STR.
You would need this patch queue, with the last "fix" changeset:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ac9b01a8578cc0a35344494a70cf8c38f9f7f29
And run this test:
./mach mochitest --headless devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js --repeat 10

This might relate to registering/unregistering one JSWindowActor multiple times.

This crash prevents me from landing bug 1620243, which is a significant stone for Fission support in DevTools.

Please have a look at comment 0 for context.
I will be happy to provide a patch for this, with some mentoring on what is the best option.
Also, feel free to redirect the ni?!

Flags: needinfo?(nika)

nsWindowRoot never clears its mWindow field to nullptr after it's created, and window root objects are always created with an initial window member. The only way that mWindow should ever be nullptr would be if the nsWindowRoot has been unlinked by the cycle-collector, in which case it should theoretically have no outstanding references to it.

I think this is being caused by nsWindowRoot not being removed from the JSWindowActorService until ~nsWindowRoot, which means that there can still be a weak reference to it after the object has been unlinked and is invalid. We probably need to change this code to make sure to invalidate this reference in a different place.

Assignee: nobody → nika
Flags: needinfo?(nika)
Attachment #9150151 - Attachment is obsolete: true
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d1c7f77d8f01 Clear JSActorService EventListener references in Unlink, r=kmag
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: