Error when closing a Toolbox while another is still open: TypeError: can't access property "notifyTargetDestroyed", watcher is undefined
Categories
(DevTools :: Framework, defect, P3)
Tracking
(firefox103 verified)
Tracking | Status | |
---|---|---|
firefox103 | --- | verified |
People
(Reporter: jdescottes, Assigned: ochameau)
References
Details
Attachments
(3 files)
STRs:
- open tab on any page (eg wikipedia)
- open devtools
- add new tab, navigate to any page (eg mozilla.org)
- open/close devtools in this new tab
ER: No error should be logged
AR: Error logged each time you open/close devtools:
JavaScript error: resource://devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm, line 217: TypeError: can't access property "notifyTargetDestroyed", watcher is undefined
Reporter | ||
Comment 1•3 years ago
|
||
FWIW, the watcher was unregistered from the registry via:
unregisterWatcher@resource://devtools/server/actors/watcher/WatcherRegistry.jsm:236:17
destroy@resource://devtools/server/actors/watcher.js:177:21
destroy@resource://devtools/shared/protocol/Pool.js:211:17
destroy@resource://devtools/shared/protocol/Actor.js:76:11
destroy@resource://devtools/server/actors/descriptors/tab.js:245:29
destroy@resource://devtools/shared/protocol/Pool.js:211:17
destroy@resource://devtools/server/actors/root.js:195:36
destroy@resource://devtools/shared/protocol/Pool.js:211:17
onTransportClosed/<@resource://devtools/server/devtools-server-connection.js:493:34
onTransportClosed@resource://devtools/server/devtools-server-connection.js:493:19
close@resource://devtools/shared/transport/local-transport.js:170:22
close@resource://devtools/shared/transport/local-transport.js:165:13
close@resource://devtools/client/devtools-client.js:131:23
destroy@resource://devtools/client/fronts/descriptors/descriptor-mixin.js:71:22
Feels really weird that this only happens if you have another toolbox open though. But I notice that when I only have one toolbox, I see JavaScript error: , line 0: NotFoundError: No such JSWindowActor 'DevToolsFrame'
instead, so maybe we fail earlier, because we already unregistered the actors.
Assignee | ||
Comment 2•3 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #1)
Feels really weird that this only happens if you have another toolbox open though. But I notice that when I only have one toolbox, I see
JavaScript error: , line 0: NotFoundError: No such JSWindowActor 'DevToolsFrame'
instead, so maybe we fail earlier, because we already unregistered the actors.
I investigated this one first as it happens on the simplest STR. Open and close DevTools.
It relates to this request:
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/actors/watcher/target-helpers/frame-helper.js#176-181
browsingContext.currentWindowGlobal
.getActor("DevToolsFrame")
.destroyTarget({
watcherActorID: watcher.actorID,
sessionContext: watcher.sessionContext,
});
Unfortunately, it doesn't throw from here. This is why we have an unhandled error with no stack.
I suspect it throws from here, when we receive the message in the content process, but the JSWindowActor has been unregistered in between.
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/dom/ipc/jsactor/JSActorManager.cpp#174
The JSWindowActor is being unregistered from here:
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/actors/watcher/WatcherRegistry.jsm#364
destroyTarget
is being called first with the following stack:
destroyTargets
destroyTargets@resource://devtools/server/actors/watcher/target-helpers/frame-helper.js:168:8
unwatchTargets@resource://devtools/server/actors/watcher.js:240:24
destroy@resource://devtools/server/actors/watcher.js:174:12
destroy@resource://devtools/shared/protocol/Pool.js:211:17
destroy@resource://devtools/shared/protocol/Actor.js:76:11
destroy@resource://devtools/server/actors/descriptors/tab.js:245:29
While JSWindowActor is being unregistered after, with the following stack:
unregisterJSWindowActor@resource://devtools/server/actors/watcher/WatcherRegistry.jsm:363:8
maybeUnregisteringJSWindowActor@resource://devtools/server/actors/watcher/WatcherRegistry.jsm:297:7
unregisterWatcher@resource://devtools/server/actors/watcher/WatcherRegistry.jsm:235:10
destroy@resource://devtools/server/actors/watcher.js:178:21
destroy@resource://devtools/shared/protocol/Pool.js:211:17
destroy@resource://devtools/shared/protocol/Actor.js:76:11
destroy@resource://devtools/server/actors/descriptors/tab.js:245:29
This boils down to WatcherActor.destroy:
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/actors/watcher.js#169-181
destroy: function() {
// Force unwatching for all types, even if we weren't watching.
// This is fine as unwatchTarget is NOOP if we weren't already watching for this target type.
for (const targetType of Object.values(Targets.TYPES)) {
this.unwatchTargets(targetType); // <============= this is where we call destroyTarget()
}
this.unwatchResources(Object.values(Resources.TYPES));
WatcherRegistry.unregisterWatcher(this); // <========= this is where we unregister the JSWindowActor
destroyTarget
uses sendAsyncMessage
as we didn't want to make the destroy codepath async.
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm#71-76
And so because of that we send the async message and right after, in the same event loop, ask to unregister the JSWindowActor.
And it looks like the unregister request takes the precedance over our target destruction message :x
This exception doesn't highlight anything wrong. If it throws, it means that the JSWindowActorChild will be destroyed and will clean itself up.
So it isn't an issue to miss the destroy target message.
We could:
- make WatcherActor.destroy become async, use a query and wait for the query completion before unregistering the watcher. But we learned that having async destroy brings some other issues...
- introduce a small delay before actually unregistering the JSWindowActor?
- stop unregistering the JSWindowActor. But we should probably try to optimize them even more as they would be loaded for any new WindowGlobal...
- unfortunately, as the exception happens from C++ layer it is impossible to silence this unharmful exception
- revive this patch in order to load the JSWindowActor only if the BrowsingContext is having
watchedByDevTools==true
. But this requires something else for the browser toolbox. - ???
Assignee | ||
Comment 3•3 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #0)
STRs:
- open tab on any page (eg wikipedia)
- open devtools
- add new tab, navigate to any page (eg mozilla.org)
- open/close devtools in this new tab
ER: No error should be logged
AR: Error logged each time you open/close devtools:JavaScript error: resource://devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm, line 217: TypeError: can't access property "notifyTargetDestroyed", watcher is undefined
This one is related to the exact same code. But should be simplier to handle.
When we call destroyTarget
, in the content process, we will destroy the target.
Once the target is destroyed, we will emit the DevToolsFrameChild:destroy
message from here:
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm#238-251
Which is correctly received by the parent process in the function that throws:
https://searchfox.org/mozilla-central/rev/c8e15e17bc6fd28f558c395c948a6251b38774ff/devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm#214-219
case "DevToolsFrameChild:destroy":
for (const { form, watcherActorID } of message.data.actors) {
const watcher = WatcherRegistry.getWatcher(watcherActorID);
watcher.notifyTargetDestroyed(form); // <========== `watcher` is null here
}
return this._closeAllConnections();
And so watcher is null here, for the same reason as the previous comment.
In WatcherActor.destroy
, we call destroyTargets just before unregistering the watcher from WatcherRegistry:
destroy: function() {
// Force unwatching for all types, even if we weren't watching.
// This is fine as unwatchTarget is NOOP if we weren't already watching for this target type.
for (const targetType of Object.values(Targets.TYPES)) {
this.unwatchTargets(targetType); // <=== this is where we will trigger target destruction and ultimately emit the "DevToolsFrameChild:destroy" message
}
this.unwatchResources(Object.values(Resources.TYPES));
WatcherRegistry.unregisterWatcher(this); // <========= this is where we unregister the watcher from WatcherRegistry
In this case, I believe it is fine to ignore "DevToolsFrameChild:destroy" message for unregistered watchers.
Assignee | ||
Comment 4•3 years ago
|
||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
Thanks to the exception fixed by the previous patch of this queue,
we weren't triggering closeAllConnections
!
Calling this method was simply all wrong.
It was closing all watcher's connections intead of only the one
related to the currently destroyed target.
Doing this was breaking cases involving more than one watcher per server.
When opening RDM and a Toolbox. Closing one of the two, would destroy the other one's connection/actors/fronts.
Assignee | ||
Comment 8•3 years ago
|
||
The _connections
map is still not quite the same between the two,
and workers auto-destroy the JSWindowActor when the last connection closes.
But this auto-destroy looks meaningless? If the map is empty _destroy won't do much...
Regarding merging or sharing code between the two actors,
I don't have a clear picture on how to do so as there is subtle differences.
It might be clearer once content process targets are using a JSProcessActor.
It would introduce a third Actor, doing probably the same things again, in a yet different way!
Comment 10•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5ad39cb599c
https://hg.mozilla.org/mozilla-central/rev/14b357fae81e
Comment 11•3 years ago
|
||
bugherder |
Updated•3 years ago
|
I managed to reproduce this issue on a 2022-06-01 Nightly build on macOS 11. Verified as fixed on Firefox 103.0b4(build ID: 20220703190044) and Nightly 104.0a1(build ID: 20220704214252) on macOS 11, Ubuntu 22.04 and Windows 10 64-bits.
Description
•