Bogdan Maris, Desktop QA Reporter
	Description • 6 years ago

[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

	Yulia Startsev [:yulia] Assignee
	Comment 1 • 6 years ago

Thanks for the report. I can reproduce it. I will take a look at what is happening

	Yulia Startsev [:yulia] Assignee
	Comment 2 • 6 years ago

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...

	Patrick Brosset <:pbro>
	Comment 3 • 6 years ago

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

	Vlad Lucaci, Desktop QA (:vlucaci)
	Comment 4 • 6 years ago

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

	Yulia Startsev [:yulia] Assignee
	Comment 5 • 6 years ago

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

	Alexandre Poirot [:ochameau]
	Comment 6 • 6 years ago

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.

	David Durst [:ddurst]
	Comment 7 • 6 years ago

Unless the bugs filed in #c6 are going to land in time, we should back this out?

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 8 • 6 years ago

Forwarding this to Yulia

	Yulia Startsev [:yulia] Assignee
	Comment 9 • 6 years ago

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

	Yulia Startsev [:yulia] Assignee
	Comment 11 • 6 years ago

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f104d7125bd060db000ee29192b7ff60e4ac76

	Pulsebot
	Comment 12 • 6 years ago

Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81624993c03d
use custom memoization for the inspector; r=ochameau

	Dorel Luca [:dluca]
	Comment 13 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/81624993c03d

	Emil Ghitta, QA [:emilghitta]
	Comment 14 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

Is this worth backporting to Beta with the Fx63 RC build happening next Monday?

	Yulia Startsev [:yulia] Assignee
	Comment 16 • 6 years ago

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.

	Yulia Startsev [:yulia] Assignee
	Comment 17 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 18 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-release/rev/a8e9539928ac

	Cristian Baica [:cbaica], Release Desktop QA
	Comment 20 • 6 years ago

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.