Closed Bug 1665020 Opened 8 months ago Closed 11 days ago

DevTools Toolbox broken when navigating cross origin

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(Fission Milestone:M7a, firefox90 fixed)

RESOLVED FIXED
90 Branch
Fission Milestone M7a
Tracking Status
firefox90 --- fixed

People

(Reporter: ochameau, Assigned: nchevobbe)

References

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1664438 +++

From Julian:

Seems to be triggered when navigating while the toolbox is not fully ready.

STRs:

  1. open https://www.wikipedia.org/
  2. Open toolbox (inspector or debugger seem to reproduce the bug)
  3. Navigate to mozilla.org
  4. Navigate back and forth between the two (with back/forward button) until the current devtools panel breaks: empty source pane for the debugger, empty markup view for the inspector. This requires a specific timing, not too quick, not too fast (but quite easy to repro IMO)
  5. At that point, the panel will remain broken, and if you trigger the DevTools shortcut, a new Toolbox will open on top of the existing one.

Bug 1664438 is going to fix very frequent failures, but some other exceptions, with the exact same STR, lead to the same issue at the end:

console.error: "Error while calling actor 'inspector's method 'getHighlighter'" "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMWindowUtils.getRootBounds]"
console.error: "getWindowDimensions@resource://devtools/shared/layout/utils.js:599:39\nAutoRefreshHighlighter@resource://devtools/server/actors/highlighters/auto-refresh.js:81:25\nBoxModelHighlighter@resource://devtools/server/actors/highlighters/box-model.js:101:5\n_createHighlighter@resource://devtools/server/actors/highlighters.js:144:25\ninitializeInstance@resource://devtools/server/actors/highlighters.js:137:12\ngetHighlighter/this._highlighterPromise<@resource://devtools/server/actors/inspector/inspector.js:208:25\n"
JavaScript error: resource://devtools/shared/protocol/Front.js, line 335: Error: Protocol error (NS_ERROR_UNEXPECTED): Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMWindowUtils.getRootBounds] from: server0.conn0.child4162/inspectorActor3 (resource://devtools/shared/layout/utils.js:599:0)

Tracking dt-fission-m2-reserve bugs for Fission Beta milestone (M7).

Fission Milestone: --- → M7

Bulk move of all dt-fission-m2-reserve bugs to Fission MVP milestone.

Fission Milestone: M7 → MVP
Severity: -- → S3
Whiteboard: dt-fission-m2-reserve → dt-fission-m3-mvp

STR from comment 0 still reproduces, but probably with new set of exceptions:

JavaScript error: resource://devtools/shared/resources/target-list.js, line 540: TypeError: can't access property "client", targetFront is null
console.error: "Tried to call watchFronts for the 'inspector' type on an already destroyed front 'browsingContextTarget'."
console.error: "Error while listing frames" (new Error("Can not send request 'listFrames' because front 'browsingContextTarget' is already destroyed.", "resource://devtools/shared/protocol/Front/FrontClassWithSpec.js", 28))
console.warn: "Failed to add watcher data entry for frame targets in browsing context" 46
console.warn: (new Error("No target actor for this Watcher Actor ID:\"server0.conn1.watcher4\" / BrowserId:9", "resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm", 407))

And this one seems to be the one reproducing when it breaks:

JavaScript error: resource://devtools/shared/resources/target-list.js, line 540: TypeError: can't access property "client", targetFront is null

Note that many exceptions occur when doing cross process navigation, but most of them do not break the tools, or at least not in a very visible way.

Moving some dt-fission-m3-mvp bugs from Fission MVP to M7 (blocking Beta experiment).

Fission Milestone: MVP → M7
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED

I tried to add a test for this, and it does fail quite a lot :)
One immediate thing that we can see break is the toolbox toolbar: the isChecked call for the RDM button throws because toolbox.target.localTab throws too. This is easily fixable by adding a try/catch block around that and assume RDM is closed when it throws.

This seems to highlight a more common issue though, where the DevToolsClient gets destroyed. This shouldn't happen during the target switch, as we set a shouldCloseClient flag to false on the target (https://searchfox.org/mozilla-central/rev/644e42ded761d4f3ce108fa776197730a9a2c535/devtools/shared/resources/target-list.js#545). But it looks like we may not go through this in some cases (that I still need to investigate)

Attachment #9206016 - Attachment description: Bug 1665020 - [devtools] Add a test for checking Toolbox state after multiple backward/forward navigations. → Bug 1665020 - [devtools] Add a test for checking Toolbox state after multiple backward/forward navigations. r=ochameau,jdescottes.

Accessing toolbox.target.localTab can throw in some specific cases, if we're in
the middle of a navigation.
This could break the rendering of the Toolbox toolbar when doing a target switch.
Wrapping the call in a try/catch block is enough to fix the issue, and the only
downside is that we wouldn't check the RDM icon in the toolbar.

Depends on D106750

This adds multiple guards so we don't call functions that would trigger errors
further down the road (e.g. an onResourceAvailable callback whereas the associated target
is already destroyed).
This isn't enough to make browser_toolbox_backward_forward_navigation.js pass,
but this does prevent some errors from appearing.

Depends on D107658

Attachment #9207781 - Attachment description: Bug 1665020 - [devtools] Wrap RDM `isChecked` definition in try/catch. r=ochameau. → Bug 1665020 - [devtools] Access local tab from the toolbox descriptorFront instead of its target in RDM definition. r=ochameau.
Attachment #9207782 - Attachment description: Bug 1665020 - [devtools] Check watcher/target front aren't destroyed before calling functions in resource watcher. r=ochameau. → Bug 1665020 - [devtools] Don't trigger onResourceAvailable/Updated for resources bound to destroyed targets. r=ochameau.
Depends on: 1697450
Depends on: 1697452
Depends on: 1697453

Comment on attachment 9206016 [details]
Bug 1665020 - [devtools] Add a test for checking Toolbox state after multiple backward/forward navigations. r=ochameau,jdescottes.

Revision D106750 was moved to bug 1697450. Setting attachment 9206016 [details] to obsolete.

Attachment #9206016 - Attachment is obsolete: true

Comment on attachment 9207781 [details]
Bug 1665020 - [devtools] Access local tab from the toolbox descriptorFront instead of its target in RDM definition. r=ochameau.

Revision D107658 was moved to bug 1697452. Setting attachment 9207781 [details] to obsolete.

Attachment #9207781 - Attachment is obsolete: true

Comment on attachment 9207782 [details]
Bug 1665020 - [devtools] Don't trigger onResourceAvailable/Updated for resources bound to destroyed targets. r=ochameau.

Revision D107659 was moved to bug 1697453. Setting attachment 9207782 [details] to obsolete.

Attachment #9207782 - Attachment is obsolete: true

Once Bug 1697450 lands, this test should be about enabling devtools/client/framework/test/browser_toolbox_backward_forward_navigation.js on fission.

Note that work for Bug 1694906 might help

Depends on: 1694906

Bug 1631451 will remove shouldCloseClient, which will hopefully fix the most frequent error we see in browser_toolbox_backward_forward_navigation.js (client being destroyed while doing a target switch)

Depends on: 1631451

Question: Since Bug 1694651 references BFCache, it was moved downstream to a M7a blocker. Will that present a problem for closing this bug ( Bug 1665020) ?

Flags: needinfo?(nchevobbe)

(In reply to Frank Griffith from comment #14)

Question: Since Bug 1694651 references BFCache, it was moved downstream to a M7a blocker. Will that present a problem for closing this bug ( Bug 1665020) ?

No this won't, since Alex created a specific bug for the part that would help here (Bug 1631451)
I edited my comment where I was referencing Bug 1694651 to avoid confusion

Flags: needinfo?(nchevobbe)
Depends on: 1697747
Fission Milestone: M7 → M8
Depends on: 1698565
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8791e5e22af7
[devtools] Enable browser_toolbox_backward_forward_navigation.js on fission. r=jdescottes.
Status: ASSIGNED → RESOLVED
Closed: 11 days ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Fission Milestone: M8 → M7a
You need to log in before you can comment on or make changes to this bug.