Closed Bug 1493131 Opened Last year Closed Last year

Inspector console is broken after loading a website inside a locally saved page

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 verified, firefox64 verified)

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- verified
firefox64 --- verified

People

(Reporter: bogdan_maris, Assigned: yulia)

References

Details

(Keywords: regression)

Attachments

(2 files)

[Affected versions]:
- Firefox 63.0b8
- Latest Nightly 64.0a1

[Unaffected versions]:
- Firefox 62.0.2

[Affected platforms]:
- Windows 10 64bit
- Ubuntu 18.04 64bit
- macOS 10.13

[Steps to reproduce]:
1. Start Firefox
2. Open in url bar a local saved pdf (eg. https://bugzilla.mozilla.org/attachment.cgi?id=8792203 - download, save and open from local drive) 
 - not sure if a specific format is required but I used a pdf
3. Right click the page and select Inspect Element
4. Click Pick an element from the page icon (Top left icon)
5. Hover over a few elements in the page
6. Focus url bar and type/visit a random webpage (eg https://reddit.com) 

[Expected result]:
- Inspector is not broken

[Actual result]:
- Inspector is broke, many elements are missing. Opening another instance of the inspector will result in having the garbage from the first inspector left in the page and a new instance of the inspector will open.

[Regression range]:
- This is a recent regression.
Last good revision: d90dcb1712d4957676643616337e420d197a83c9
First bad revision: 6c83f735355d19458caa7ff34069b5676c062228
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d90dcb1712d4957676643616337e420d197a83c9&tochange=6c83f735355d19458caa7ff34069b5676c062228

Maybe this can be the culprit? Bug 1447487 - Stop using gcli for eyedropper

[Additional notes]:
- Following the steps multiple times will result in having many broken inspector consoles on top of each other.
- Here is also a screencast showing the issue: https://drive.google.com/open?id=1VJmiHI2X33QMvvOPWdUbTjNBG3cK5tJ4
Flags: needinfo?(ystartsev)
Thanks for the report. I can reproduce it. I will take a look at what is happening
Flags: needinfo?(ystartsev)
Priority: -- → P2
Assignee: nobody → ystartsev
I am not longer able to reproduce this on any revision, which is strange because I had reproduced it just before the weekend... Is there any more information about reproducing this? I think it is an issue in the destruction path that was introduced with getFront but it is hard to say...
Flags: needinfo?(bogdan.maris)
Flags: needinfo?(jdescottes)
Flags: needinfo?(jdescottes)
I reproduced the issue locally on macOS with Firefox 63.
When the problem happens, here's the stacktrace I can see in the browser console

Tried to send a 'tabDetached' event on an already destroyed actor 'frameTarget' protocol.js:990:7
_sendEvent
resource://devtools/shared/protocol.js:990:7
Actor/<
resource://devtools/shared/protocol.js:971:9
emit
resource://devtools/shared/event-emitter.js:178:15
emit
resource://devtools/shared/event-emitter.js:255:5
_detach
resource://devtools/server/actors/targets/browsing-context.js:908:5
exit
resource://devtools/server/actors/targets/browsing-context.js:517:5
frameTargetPrototype.exit
resource://devtools/server/actors/targets/frame.js:80:3
destroy
resource://devtools/server/actors/targets/browsing-context.js:499:5
removeActor
resource://devtools/server/actors/common.js:242:7
APDestroy
resource://devtools/server/actors/common.js:204:7
onClosed/<
resource://devtools/server/main.js:1820:35
onClosed
resource://devtools/server/main.js:1820:5
close
resource://devtools/shared/transport/child-transport.js:62:5
close
resource://devtools/server/main.js:1431:7
onDisconnect<
resource://devtools/server/startup/frame.js:122:7
exports.makeInfallible/<
resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
Hello all,

In order to answer Comment 2:

Yes, we are still able to reproduce this issue with the exact same STR as Bogdan has described:

1. Start Firefox
2. Open in url bar a local saved pdf (eg. https://bugzilla.mozilla.org/attachment.cgi?id=8792203 - download, save and open from local drive) 
3. Right click the page and select Inspect Element
4. Click Pick an element from the page icon (Top left icon)
5. Hover over a few elements in the page
6. Focus url bar and type/visit a random webpage (eg https://reddit.com) 
 
We can reproduce this issue on the exact same platforms on the following builds:

-63b10 (build id: 20180927143327)
-Latest Nightly 64.0a1 (build id: 20180927220034)

Platforms on which this issue was reproduced: 

-Windows 10x64
-macOSX 10.13.6
-Ubuntu 16.04

For more information, please refer to the newly added attachment:
https://drive.google.com/file/d/1ssioD5akDjHIgTMkpQJOeG6Z1lkkFtsA/view?usp=sharing
Flags: needinfo?(bogdan.maris)
It doesn't seem to be related to the eyedropper code because that codepath is never called with these steps, you have to select the eyedropper in order for those changes to have any impact, and that is in a different menu. The tabDetatched error is likely not related, as this was a silent error that was made explicit in bug #1026583, and occurs even if everything is normal.

I did track it down to a change set. The issue was with migrating to getFront: https://hg.mozilla.org/mozilla-central/rev/c3a6cf6cf350 Reverting this seems to bring back the old behavior. Unfortunately this points to a larger problem. The inspector is intertwined with the toolbox code, specifically its instantiation (https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#2655-2680) and destruction (https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#2730-2778). The problem seems to be that the inspector is now being destroyed out of order with the highlighters. This means it has extra requirements outside of the getFront cycle as it is now, which instantiates (https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#365-373) and destroys (https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#679-681) for us. To get this right, it will take some work, and up to now I haven't found a quick fix for this bug. We can't go back to the old behavior as it isn't fission compatible, we will need to solve this properly before going forward. At least, this is how far I got today, I will keep investigating when I get back from holidays
We might want to revert bug 1485374 until this is figured out.

I didn't catched up while reviewing bug 1485374, but when I tried converting InspectorFront to target.getFront in bug 1222047, I was having various failure on try, especially mochitest document leaks. See bug 1222047 comment 21:
> It is simple for stylesheet as there is nothing special being made on instanciation/destruction.
> It is slightly more complex for preference, as we still want to clear the preferences being set by the toolbox, but no longer manually destroy the front.
> It is quite straightforward for device. We just have to tweak WebIDE as creating the fronts is now asynchronous (getRootFront retursn a promise).
> It is much more complex for performance as we do many things in toolbox.js on opening/close. I would like to followup on this in order to move the logic into the front rather than polluting the toolbox codebase with panel/front specifics.
> It is even more complex for the inspector as changing anything to it introduce docshell leaks in mochitests. I'll keep inspector for a dedicated followup.

I opened bug 1495388 to cleanup inspector setup/destroy code which depends on bug 1495387 and bug 1495386.
Depends on: 1485374
Unless the bugs filed in #c6 are going to land in time, we should back this out?
Flags: needinfo?(gl)
Forwarding this to Yulia
Flags: needinfo?(gl) → needinfo?(ystartsev)
We won't be able to back out this change as it was the prerequiste for the eyedropper GCLI change in bug #1447487. I think we will just need to take a different approach for now until we can resolve the inspector issue for getFront
Flags: needinfo?(ystartsev)
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81624993c03d
use custom memoization for the inspector; r=ochameau
https://hg.mozilla.org/mozilla-central/rev/81624993c03d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Flags: qe-verify+
I have managed to reproduce this issue using Firefox 64.0a1 (BuildId:20180921220134) on Windows 10 64bit.

This issue is no longer reproducible using Firefox 64.0a1 (BuildId:20181008100121) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Is this worth backporting to Beta with the Fx63 RC build happening next Monday?
Flags: needinfo?(ystartsev)
I dont know if there will be time to do it still... but this would be an easy fix. The code that is added is not much and there is not much risk of it breaking.
Flags: needinfo?(ystartsev)
Comment on attachment 9014335 [details]
Bug 1493131 - use custom memoization for the inspector; r=ochameau

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493131

User impact if declined: Inspector may break if a user is debugging on a local tab, opens a highlighter, and without closing the highlighter navigates to another tab.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: [Steps to reproduce]:
1. Start Firefox
2. Open in url bar a local saved pdf (eg. https://bugzilla.mozilla.org/attachment.cgi?id=8792203 - download, save and open from local drive) 
 - not sure if a specific format is required but I used a pdf
3. Right click the page and select Inspect Element
4. Click Pick an element from the page icon (Top left icon)
5. Hover over a few elements in the page
6. Focus url bar and type/visit a random webpage (eg https://reddit.com) 

[Expected result]:
- Inspector is not broken

[Actual result]:
- Inspector is broke, many elements are missing. Opening another instance of the inspector will result in having the garbage from the first inspector left in the page and a new instance of the inspector will open.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): not risky

String changes made/needed: no
Attachment #9014335 - Flags: approval-mozilla-release?
Comment on attachment 9014335 [details]
Bug 1493131 - use custom memoization for the inspector; r=ochameau

Fix on nightly for +10 days, good code coverage, limited in scope and small patch, let's take this for 63rc2, thanks.
Attachment #9014335 - Flags: approval-mozilla-release? → approval-mozilla-release+
I have managed to reproduce the issue using Fx64.0a1 buildID: 20180921220134.
The issue was verified fixed using Fx 63.0-RC build 2 and the latest nightly Fx64.0a1 on mac OS 10.13.6, win 10 x64 and ubuntu 16.04 LTS x64. The inspect element panels are correctly displayed when switching pages.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.