TabDescriptor leak the first top level target
Categories
(DevTools :: Framework, defect, P2)
Tracking
(firefox95 fixed)
Tracking | Status | |
---|---|---|
firefox95 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-perf-stability-triage)
Attachments
(7 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1728072 - [devtools] Ensure that WindowGlobal._originalWindow is set immediatly (before attach).
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
The top level target is leaked by the following _targetFrontPromise
promise which is never nullified:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/devtools/client/fronts/descriptors/tab.js#108-110
Note that the following nullication never happens in case of server side targets:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/devtools/client/fronts/descriptors/tab.js#275
as _targetFrontPromise
is always set early and we bail out early in getTarget
.
Assignee | ||
Comment 1•3 years ago
|
||
Comment 3•3 years ago
|
||
Backed out for causing failures at browser_resources_document_events.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/28f52a71e290406f6258d9ed9f44db065a83e0b5
Failure log: https://treeherder.mozilla.org/logviewer?job_id=351687655&repo=autoland&lineNumber=35616
Assignee | ||
Comment 4•3 years ago
|
||
Unfortunately, this highlight existing issue with will-navigate which aren't trivial to fix:
https://phabricator.services.mozilla.com/D123925#4083939
Comment 5•3 years ago
|
||
There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ochameau, could you have a look please?
For more information, please visit auto_nag documentation.
Comment 6•3 years ago
|
||
(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #5)
There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ochameau, could you have a look please?
For more information, please visit auto_nag documentation.
I think there was some test failing still (See Comment 4)
Assignee | ||
Comment 7•3 years ago
|
||
Without this many tests start failing in the next changeset
because will-navigate's target front is destroyed before the frontend
process DOCUMENT_EVENT's will-navigate.
Assignee | ||
Comment 8•3 years ago
|
||
By matching via BrowsingContext's ID, we can match the next Target Front,
of the page we navigate to. That, instead of matching the previous Target Front,
of the page we navigate from.
Using innerWindowId allows to match a unique target actor/front.
Assignee | ||
Comment 9•3 years ago
|
||
Otherwise, it may be null if we query this attribut in between target instantiation and the time we attach it.
Assignee | ||
Comment 10•3 years ago
|
||
These IDs should be fixed to the top level WindowGlobal that the target is debugging.
We somewhat fake changing to another WindowGlobal to implement the iframe dropdown
(via WindowGlobal.setWindow) but this is only transient and doesn't change
the fact that the current target actor is meant to debug its original top level WindowGlobal.
Not doing this confuses the TargetCommand which is trying to look for a target actor
for the iframe we switched to, which doesn't exists as it doesn't have any specific target actor.
Assignee | ||
Comment 11•3 years ago
|
||
Finally, a green try for this:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=MW5yBzyiSUOGOx-fG9ausg.0&revision=27612e715740f70da14b4297251dcefba19b6e48
This was quite a quest to get this right.
Assignee | ||
Comment 12•3 years ago
|
||
browser_target_command_frames.js covers explicit destruction order of iframes and started failing
because of the code to delay target-destroyed to be fired after will-navigate for top targets.
Assignee | ||
Comment 13•3 years ago
|
||
This patch helps fix browser_grids_grid-list-on-target-added-removed.js when applying previous patches of this queue.
Slightly delay the precise timing when we emit target-destroyed.
This introduce a race condition in getAllFronts, which most likely cache
a list of target fronts before receiving the target-destroyed event,
then process the top level target asynchronously,
then try to process the iframe target which has been destroyed in the meantime.
Trying to call TargetFront.getFront on this destroyed target throws
and ultimately break the grid inspector which will reset its view from GridInspector._updateGridPanel.
Comment 14•3 years ago
|
||
Updated•3 years ago
|
Comment 15•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cfdbb047cd30
https://hg.mozilla.org/mozilla-central/rev/664cba0c89fa
https://hg.mozilla.org/mozilla-central/rev/09149d4b1e28
https://hg.mozilla.org/mozilla-central/rev/95de1b946ed1
https://hg.mozilla.org/mozilla-central/rev/3805f9fbdbad
https://hg.mozilla.org/mozilla-central/rev/639009942941
https://hg.mozilla.org/mozilla-central/rev/344174ae9995
Description
•