Closed Bug 1728072 Opened 3 years ago Closed 3 years ago

TabDescriptor leak the first top level target

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(firefox95 fixed)

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-perf-stability-triage)

Attachments

(7 files)

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.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6f72dd6aadb [devtools] Prevent leaking the first top level target. r=nchevobbe

Unfortunately, this highlight existing issue with will-navigate which aren't trivial to fix:
https://phabricator.services.mozilla.com/D123925#4083939

Flags: needinfo?(poirot.alex)

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.

Flags: needinfo?(poirot.alex)
Flags: needinfo?(nchevobbe)

(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)

Flags: needinfo?(nchevobbe)

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.

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.

Otherwise, it may be null if we query this attribut in between target instantiation and the time we attach it.

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.

Flags: needinfo?(poirot.alex)

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.

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.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cfdbb047cd30 [devtools] Delay target-destroyed to be emitted *after* will-navigate. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/664cba0c89fa [devtools] Ensure that WindowGlobal._originalWindow is set immediatly (before attach). r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/09149d4b1e28 [devtools] WindowGlobalTarget's form's BrowsingContext id and innerWindowId shouldn't be affected by the iframe dropdown. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/95de1b946ed1 [devtools] Use innerWindowId to match DOCUMENT_EVENT's will-navigate's target front. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/3805f9fbdbad [devtools] Ensure destroying all children iframes targets before destroying their top target. r=bomsy https://hg.mozilla.org/integration/autoland/rev/639009942941 [devtools] Prevent leaking the first top level target. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/344174ae9995 [devtools] Avoid TargetFront.getAllFront to throw when a target is destroyed while it is processing. r=nchevobbe
Severity: -- → S3
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: