1578745 - Introduce a preference to enable fission frame inspection in DevTools

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

We should introduce a preference similar to devtools.browsertoolbox.fission, but this time to enable Fission frame debugging directly from regular web toolboxes.

This will depend on bug 1565200 as nothing can work against fission frame until we make connectToFrame to work against Fission frames, via the JS Window Actor API.

A good first usecase to enable this preference would be to also iterate over child inspector front in this code:
https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/devtools/shared/fronts/inspector.js#573-577

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

When implementing this, we might want to do some modification in inspector front's getNodeFrontFromNodeGrip (check the TODO there)

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)

When implementing this, we might want to do some modification in inspector front's getNodeFrontFromNodeGrip (check the TODO there)

Can you give a bit more details of what you expect to be done here? Thanks!

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #2)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)

When implementing this, we might want to do some modification in inspector front's getNodeFrontFromNodeGrip (check the TODO there)

Can you give a bit more details of what you expect to be done here? Thanks!

We were saying in reviews that we should also enter this if block fronts/inspector.js#655-661 if the preference isn't true.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1578745 - Introduce a preference to enable fission frame inspection in DevTools. r=nchevobbe

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94a89e567444
Introduce a preference to enable fission frame inspection in DevTools. r=nchevobbe

Comment 6

[Arthur Iakab [arthur_iakab]]

Backed out changeset 94a89e567444 (bug 1578745) for causing multiple devtools failures on browser_dbg-inline-preview.js. CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/d33a6058d98bb3508d237525e15bcb992e9e9315

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=94a89e567444c67a68d4e237356f0a6c85461857

Failure logs: dt7 - https://treeherder.mozilla.org/logviewer.html#?job_id=273326721&repo=autoland
dt4 - https://treeherder.mozilla.org/logviewer.html#?job_id=273326725&repo=autoland
dt3 - https://treeherder.mozilla.org/logviewer.html#?job_id=273333922&repo=autoland
dt2 - https://treeherder.mozilla.org/logviewer.html#?job_id=273326718&repo=autoland

Gabriel can you please take a look?

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Arthur Iakab [arthur_iakab] from comment #6)

Backed out changeset 94a89e567444 (bug 1578745) for causing multiple devtools failures on browser_dbg-inline-preview.js. CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/d33a6058d98bb3508d237525e15bcb992e9e9315

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=94a89e567444c67a68d4e237356f0a6c85461857

Failure logs: dt7 - https://treeherder.mozilla.org/logviewer.html#?job_id=273326721&repo=autoland
dt4 - https://treeherder.mozilla.org/logviewer.html#?job_id=273326725&repo=autoland

Gabriel can you please take a look?

This turned out to be a keyboard typo in the constants that must've happened while making the review changes.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7210449434e
Introduce a preference to enable fission frame inspection in DevTools. r=nchevobbe

Comment 9

[Arthur Iakab [arthur_iakab]]

Also it seem that some browser chrome tests turned perma just before the backout:

bc16 - https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=0a516296693bb8b221744bae054419be6f2510c2&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-16%2Cm-fis%28bc16%29&selectedJob=273342852

bc3 - https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=273346452&fromchange=0a516296693bb8b221744bae054419be6f2510c2&searchStr=Windows%2C10%2Cx64%2Cdebug%2CMochitests%2Ctest-windows10-64%2Fdebug-mochitest-browser-chrome-e10s-3%2CM%28bc3%29

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d7210449434e

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9104346 [details]
Bug 1578745 - Introduce a preference to enable fission frame inspection in DevTools. r=nchevobbe

Beta/Release Uplift Approval Request

• User impact if declined: Users can't jump from a logged dom element in the console to the inspector if said element is in an iframe.
  This patch wasn't made to fix anything, but appears to fix Bug 1591757.

An automated test will be added in Bug 1591757 and I'll request the uplift as well there to ensure everything is okay on Beta.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See https://bugzilla.mozilla.org/show_bug.cgi?id=1591757#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch introduce a new preference, that is false by default.
  The fact that the preference wasn't set was the cause of the issue of Bug 1591757.
• String changes made/needed:

Comment 12

[Chris Peterson [:cpeterson]]

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9104346 [details]
Bug 1578745 - Introduce a preference to enable fission frame inspection in DevTools. r=nchevobbe

Fix for bug 1591757 which is a tracked P1, uplift approved for 71 beta 11, thanks.

Comment 14

[Cristian Brindusan [:cbrindusan]]

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

Comment 15

[Daniel Cicas [:dcicas], Release QA]

Hello,

I can still reproduce this issue (stated in bug 1591757) on Fx 71.0b11 so I will be marking Fx 71 as affected.

Comment 16

[Glenn Linderman]

(In reply to Daniel Cicas [:dcicas], Release QA from comment #15)

Hello,

I can still reproduce this issue (stated in bug 1591757) on Fx 71.0b11 so I will be marking Fx 71 as affected.

The cases that consterned me seem to be fixed in 71 now, what case might still be affected?

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Forwarding this to nchevobbe

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Daniel Cicas [:dcicas], Release QA from comment #15)

Hello,

I can still reproduce this issue (stated in bug 1591757) on Fx 71.0b11 so I will be marking Fx 71 as affected.

Daniel, could you tell me what STR you used that fails in 71 and works in 72?

Comment 19

[Daniel Cicas [:dcicas], Release QA]

Hello,

Here are the steps I used:

1. Create a simple HTTP server with both the iframe.html and the testispector.html files on it.
2. I opened the testinspector.html file in localhost.
3. Open devtools console and click on the console.log (for lack of a better term the square next to it)

Actual result:
The inspector returns me to the body, not to the iframe nested inside several divs.

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Daniel, I just tested this in 71.0b11 (64-bit) and it works fine to me, at step 3. the h1 node of the iframe is selected in the inspector.
Could you try again please?
Maybe there's a race condition or something?

Comment 21

[Daniel Cicas [:dcicas], Release QA]

Hello,

I tried again and it's working now I must have made a slip up. I am confirming this bug as verified fixed for Fx 71.0b12 and Fx 72.0a1.