1810009 - Navigating a tab breaks the Browser Toolbox in Parent Process mode

Status: VERIFIED FIXED

(DevTools :: Framework, defect, P2)

Description

[Julian Descottes [:jdescottes]]

STRs:

• open the Browser Toolbox
• select Parent Process mode
• navigate or reload any tab in Firefox

ER: Browser Toolbox should not be severely affected
AR: Inspector becomes blank, debugger sources disappear etc...

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1772823

:ochameau, since you are the author of the regressor, bug 1772823, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Julian Descottes [:jdescottes]]

It seems that the will-navigate resources emitted by tabs are associated with a top-level target.

Comment 3

[Julian Descottes [:jdescottes]]

I think the issue is that we emit will-navigate as a parent process resource, from the watcher.

We trust resource-command to find which "target" this will-navigate corresponds to based on the browsingContextId in the resource.
However, in "parent-process" mode, we don't have a target created for individual window globals, so we fallback on the only available target: the parent process one.

In the long run I think this would be addressed by switching back to targetAvailable instead of will-navigate.

Edit: Specifically, we are retrieving the wrong target at https://searchfox.org/mozilla-central/rev/b19830c6b22f73758862d34d2c64f57735890e90/devtools/client/fronts/watcher.js#159-167. This fallback mechanism might not make sense with EFT enabled? With EFT, if we can't get a target for a given browsing context id, it means it is out of scope. But maybe here we should rather fix the server to avoid emitting will-navigate at all in this configuration

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1810009 - [devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets.

The legacy listener was explicitely avoiding emitting this event for non top-level targets.
It seems like all frontend code listening to will-navigate ignore the event is targetFront.isTopLevel is false,
so it looks like no code would expect these event.
So let's try to avoid emitting them if they aren't used by anyone.
(and can be confusing/buggy in the context of the browser toolbox)

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #3)

I think the issue is that we emit will-navigate as a parent process resource, from the watcher.

We trust resource-command to find which "target" this will-navigate corresponds to based on the browsingContextId in the resource.
However, in "parent-process" mode, we don't have a target created for individual window globals, so we fallback on the only available target: the parent process one.

In the long run I think this would be addressed by switching back to targetAvailable instead of will-navigate.

Edit: Specifically, we are retrieving the wrong target at https://searchfox.org/mozilla-central/rev/b19830c6b22f73758862d34d2c64f57735890e90/devtools/client/fronts/watcher.js#159-167. This fallback mechanism might not make sense with EFT enabled? With EFT, if we can't get a target for a given browsing context id, it means it is out of scope. But maybe here we should rather fix the server to avoid emitting will-navigate at all in this configuration

+1, hopefully making the browser toolbox use EFT will later fix all similar issues!

The legacy listener was filtering out events for non-top-level targets:
https://searchfox.org/mozilla-central/rev/b19830c6b22f73758862d34d2c64f57735890e90/devtools/shared/commands/resource/resource-command.js#1237-1240
Whereas the resource watcher isn't doing so, it emits will-navigate for all "process root" targets,
so tabs and top documents of cross origin iframes.
https://searchfox.org/mozilla-central/rev/b19830c6b22f73758862d34d2c64f57735890e90/devtools/server/actors/resources/parent-process-document-event.js#111,126
It isn't clear why I let it be emitted for non-top level target. I assume it is just a mistake?

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1810009 - [devtools] Add test navigating a tab while BrowserToolbox is open

Depends on D166769

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57ac7060ddf0
[devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets. r=devtools-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/8f4d3ce7f581
[devtools] Add test navigating a tab while BrowserToolbox is open r=devtools-reviewers,nchevobbe

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out for causing multiple devtools failures related to toolbox, browser, webconsole

• backout: https://hg.mozilla.org/integration/autoland/rev/8e90be3496f15ad9aea97bebd47a5f0b6aba8b11
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=8f4d3ce7f5812fab4a9c91494bc41ce99dac559e
• failure logs:
  • TEST-UNEXPECTED-FAIL | devtools/client/inspector/grids/test/browser_grids_restored-after-reload.js | CSS grid highlighter is shown. - Got 2, expected 1
  • TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_update-on-navigtion.js | Test timed out -
  • TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_webconsole_async_stack.js | console.trace has expected frames - Got "onTimeout\n(Async: setTimeout handler)\ntimeout\nonPromiseThen", expected "onTimeout\n(Async: setTimeout handler)\ntimeout\nonPromiseThen\n(Async: promise callback)\npromiseThen\n<anonymous>"
  • TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_block-context.js | undefined assertion name - Got false, expected true

Comment 9

[Alexandre Poirot [:ochameau]]

Sorry, I had a typo in the last changeset.
Here is a new try run:
https://treeherder.mozilla.org/jobs?repo=try&revision=2c464bcf68d338a51a7cbab26ea71978f18958b6

Comment 10

[Julian Descottes [:jdescottes]]

I think you meant https://treeherder.mozilla.org/jobs?repo=try&revision=e07cba3ec16b81b3855f6ba5ffb417c220285167

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #10)

I think you meant https://treeherder.mozilla.org/jobs?repo=try&revision=e07cba3ec16b81b3855f6ba5ffb417c220285167

Yes, thanks, that other try is very orange!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9bcb8862497
[devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets. r=devtools-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/ee636c21fe18
[devtools] Add test navigating a tab while BrowserToolbox is open r=devtools-reviewers,nchevobbe

Comment 13

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures in

devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/2746049e45c5ffd3f3907aeb0a7013bcca616ef7

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js | The remote debugger process died cleanly - Got 11, expected +0

Comment 14

[Julian Descottes [:jdescottes]]

Odd I didn't have any failure for this platform on any of my try builds. This starts to be complicated to land, so let's split the test to another bug. I will reland the patch from Alex.

Comment 15

[Phabricator Automation]

Comment on attachment 9312199 [details]
Bug 1810009 - [devtools] Add test navigating a tab while BrowserToolbox is open

Revision D166796 was moved to bug 1810622. Setting attachment 9312199 [details] to obsolete.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6126dcd64f4f
[devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets. r=devtools-reviewers,jdescottes

Comment 17

[Julian Descottes [:jdescottes]]

(In reply to Sandor Molnar from comment #13)

Backed out for causing dt failures in

devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/2746049e45c5ffd3f3907aeb0a7013bcca616ef7

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js | The remote debugger process died cleanly - Got 11, expected +0

That link was only showing failures, but there are also 3 green jobs on the same push/platform: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Copt%2Cdevtools%2Cmochitests%2Cwith%2Ceft%2Cwithout%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fopt-mochitest-devtools-chrome-dt-no-eft-nofis%2Cdt3&revision=ee636c21fe180e6c41a3061d1ce237042e011af2

The intermittent here "The remote debugger process died cleanly - Got 11, expected +0" already affects other tests from the same suite: eg https://bugzilla.mozilla.org/show_bug.cgi?id=1784636.

For reference, I had 0 failures on 10 retries over at https://treeherder.mozilla.org/jobs?repo=try&revision=5a825acafbdcb51646c3f70622e538050d8a4109 for the no eft no fis job

Comment 18

[Serban Stanca [:SerbanS]]

Backed out for causing dt failures in browser_resources_document_events.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | devtools/shared/commands/resource/tests/browser_resources_document_events.js | Uncaught exception in test - at chrome://mochitests/content/browser/devtools/client/shared/test/shared-head.js:1050 - Error: Failed waitFor():

Comment 19

[Serban Stanca [:SerbanS]]

(In reply to Julian Descottes [:jdescottes] from comment #17)

(In reply to Sandor Molnar from comment #13)

Backed out for causing dt failures in

devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/2746049e45c5ffd3f3907aeb0a7013bcca616ef7

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_navigate_tab.js | The remote debugger process died cleanly - Got 11, expected +0

That link was only showing failures, but there are also 3 green jobs on the same push/platform: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Copt%2Cdevtools%2Cmochitests%2Cwith%2Ceft%2Cwithout%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fopt-mochitest-devtools-chrome-dt-no-eft-nofis%2Cdt3&revision=ee636c21fe180e6c41a3061d1ce237042e011af2

The intermittent here "The remote debugger process died cleanly - Got 11, expected +0" already affects other tests from the same suite: eg https://bugzilla.mozilla.org/show_bug.cgi?id=1784636.

For reference, I had 0 failures on 10 retries over at https://treeherder.mozilla.org/jobs?repo=try&revision=5a825acafbdcb51646c3f70622e538050d8a4109 for the no eft no fis job

Now it's also perma failing ( it can be seen after backfills )

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to sstanca from comment #19)

Now it's also perma failing ( it can be seen after backfills )

My comment was about the previous backout, for failures on browser_browser_toolbox_navigate_tab.js. I moved the test to another bug so I think your comment is rather about browser_resources_document_events.js.

Comment 21

[Serban Stanca [:SerbanS]]

(In reply to Julian Descottes [:jdescottes] from comment #20)

(In reply to sstanca from comment #19)

Now it's also perma failing ( it can be seen after backfills )

My comment was about the previous backout, for failures on browser_browser_toolbox_navigate_tab.js. I moved the test to another bug so I think your comment is rather about browser_resources_document_events.js.

Yes, sorry i didn't paid attention to that. Now it's permafailing on devtools/shared/commands/resource/tests/browser_resources_document_events.js

Comment 22

[Alexandre Poirot [:ochameau]]

The failing test is only runnig on mac while I'm on linux and the try push I used was only running only linux tests.

Here is a new multiplatform try push:
https://treeherder.mozilla.org/jobs?repo=try&revision=366664760201d09498910684aa23f543c8d16000

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1772823

Comment 24

[Alexandre Poirot [:ochameau]]

There is some intermittents, but they all look like known ones.
Trying to reland...

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/002e6dd90553
[devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets. r=devtools-reviewers,jdescottes

Comment 26

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/002e6dd90553

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:ochameau, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 28

[Alexandre Poirot [:ochameau]]

Comment on attachment 9312130 [details]
Bug 1810009 - [devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets.

Beta/Release Uplift Approval Request

• User impact if declined: The browser toolbox will clear all its content when any webpage navigates (unless the user manually changed it to be a multiprocess browser toolbox)

Requesting bug 1810622 to also be uplifted for test coverage.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: Bug 1810622
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This should only impact the browser toolbox, which is an important tool to debug firefox, but shouldn't impact regular devtools.
• String changes made/needed: no
• Is Android affected?: No

Comment 29

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: 1810009.gif

Reproduced the issue with Firefox 110.0a1 (2023-01-12) on Windows 10x64. Certain tabs inside the Browser Toolbox become blank after refreshing or navigating to a page while the Parent process is selected.
The issue is verified fixed with Firefox 111.0a1 (2023-01-18) on Windows 10x64, macOS 10.15, and Ubuntu 20.04. The tabs inside the Browser Toolbox are no longer becoming blank after refreshing or navigating to a page while the Parent process is selected.

However, while testing this I encountered two behaviors that still show a blank debugger tab after following certain steps. I cannot see the following issues with Firefox 109.0.

The first behavior will open an empty debugger after Firefox is opened with a new profile. STR:

1. Open Firefox Nightly with a new profile.
2. Open a new tab and a random webpage (e.g Wikipedia)
3. Open Web developer tools and enable Browser Toolbox.
4. Open the Browser toolbox and the Debugger tab.
   Actual result: Debbuger tab is empty on Firefox Nightly. The Debugger tab will be populated after closing and reopening the browser toolbox.

The second behavior will display an empty Debugger if the iframes are changed.

1. Open the browser toolbox.
2. Change between iframes.
   Actual result: Debugger content is erased until the browser toolbox is closed and reopened.

I have also attached a screen recording of the above-mentioned behaviors. Should we file new issues for this and close this one or this is expected? Thank you!

Comment 30

[Alexandre Poirot [:ochameau]]

Thanks a lot Alexandru for the two reports, I opened bug 1811254 (recent regression) and bug 1811257 (not clear if that's a recent regression).

Comment 31

[Alexandru Trif, Desktop QA [:atrif]]

Setting this as verified on Firefox 111.0a1 based on comment 29 since the other issues are addressed in other tickets. Thank you!

Comment 32

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9312130 [details]
Bug 1810009 - [devtools] Avoid firing DOCUMENT_EVENT will-navigate for non-top-level targets.

Approved for 110 beta 4, thanks.

Comment 33

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d15b6ea1e2b5

Comment 34

[Alexandru Trif, Desktop QA [:atrif]]

Verified fixed with Firefox 110.0b5 on Windows 10x64, macOS 12 and Ubuntu 20.04. The content of the tabs inside the Browser Toolbox no longer becomes blank after refreshing or navigating to a page while the Parent process is selected.