Crash from JSWindowActorProtocol::RegisterListenersFor against a null window
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox78 | --- | fixed |
People
(Reporter: ochameau, Assigned: nika)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Reporter | ||
Comment 1•5 years ago
|
||
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?!
Reporter | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
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 | ||
Comment 4•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Comment 6•5 years ago
|
||
bugherder |
Description
•