1460223 - Inspector shows empty body for ghacks.net

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[atlanto]

STR:
1. Go to https://www.ghacks.net/
2. Open Inspector

AR:
Body is empty.

ER:
Body displayed.

Workaround:
Reopen Inspector.

mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7f69dd1ba3cb669fdbc7db301870009087ab0639&tochange=0fcbe086a09ba9e9684f43706290b3e0f2226103

Comment 1

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on (Windows 10 x64): 

Firefox Nightly 62.0a1 (2018-05-09) (64-bit) - 20180509220105
Firefox Beta 61.0b3 – 20180507191226
Firefox Release 60.0 (64-bit) – 20180503143129
Firefox – esr60 - 20180503092946

Comment 2

[Patrick Brosset <:pbro>]

I can reproduce this problem too.
And when I do, I can see this error in the browser console:

unknownError markup.js:156:7
	_handleRejectionIfNotDestroyed resource://devtools/client/inspector/markup/markup.js:156:7
	_updateChildren resource://devtools/client/inspector/markup/markup.js:1765:7
	_expandContainer resource://devtools/client/inspector/markup/markup.js:1229:12
	expandNode resource://devtools/client/inspector/markup/markup.js:1244:12
	_deferredOpen chrome://devtools/content/inspector/inspector.js:262:13
	init chrome://devtools/content/inspector/inspector.js:169:12
	open resource://devtools/client/inspector/panel.js:12:12
	onLoad resource://devtools/client/framework/toolbox.js:1692:19

Now, if I close/re-open devtools after the page has finished loading, then the content of the body node is displayed correctly.

Also, after that, if I refresh the page with devtools opened, then the problem occurs again, and a similar stacktrace is displayed:

unknownError markup.js:156:7
	_handleRejectionIfNotDestroyed resource://devtools/client/inspector/markup/markup.js:156:7
	_updateChildren resource://devtools/client/inspector/markup/markup.js:1765:7
	_expandContainer resource://devtools/client/inspector/markup/markup.js:1229:12
	expandNode resource://devtools/client/inspector/markup/markup.js:1244:12
	onMarkupLoaded chrome://devtools/content/inspector/inspector.js:1095:20
	newListener resource://devtools/shared/event-emitter.js:137:42
	emit resource://devtools/shared/event-emitter.js:178:37
	emit resource://devtools/shared/event-emitter.js:255:29
	_onMarkupFrameLoad chrome://devtools/content/inspector/inspector.js:1824:5
	_initMarkup chrome://devtools/content/inspector/inspector.js:1810:5
	onNodeSelected chrome://devtools/content/inspector/inspector.js:1074:7
	process resource://gre/modules/Promise-backend.js:928:23
	walkerLoop resource://gre/modules/Promise-backend.js:812:7
	scheduleWalkerLoop resource://gre/modules/Promise-backend.js:745:11
	schedulePromise resource://gre/modules/Promise-backend.js:776:7
	completePromise resource://gre/modules/Promise-backend.js:713:7
	completePromise resource://gre/modules/Promise-backend.js:704:7
	process resource://gre/modules/Promise-backend.js:964:5
	walkerLoop resource://gre/modules/Promise-backend.js:812:7
	scheduleWalkerLoop resource://gre/modules/Promise-backend.js:745:11
	schedulePromise resource://gre/modules/Promise-backend.js:776:7
	completePromise resource://gre/modules/Promise-backend.js:713:7
	WalkerFront<.getMutations</< resource://devtools/shared/fronts/inspector.js:298:11
	(Async: promise callback)
	WalkerFront<.getMutations< resource://devtools/shared/fronts/inspector.js:284:12
	WalkerFront<.onMutations< resource://devtools/shared/fronts/inspector.js:432:5
	onPacket/results< resource://devtools/shared/protocol.js:1350:44
	onPacket resource://devtools/shared/protocol.js:1350:23
	onPacket resource://devtools/shared/client/debugger-client.js:860:7
	send/< resource://devtools/shared/transport/transport.js:553:13
	exports.makeInfallible/< resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
	(Async: DevToolsUtils.executeSoon)
	exports.executeSoon resource://devtools/shared/DevToolsUtils.js:57:19
	send resource://devtools/shared/transport/transport.js:547:9
	send resource://devtools/server/main.js:1476:5
	receiveMessage resource://devtools/shared/transport/transport.js:735:7
	(Async: MessageListener.receiveMessage)
	_addListener resource://devtools/shared/transport/transport.js:709:7
	ready resource://devtools/shared/transport/transport.js:726:7
	connectToFrame/</onActorCreated< resource://devtools/server/main.js:1059:9

Comment 3

[Patrick Brosset <:pbro>]

I found the root cause. Will push a fix soon.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1460223 - Handle computedStyle.display failures for non-elements;

Review commit: https://reviewboard.mozilla.org/r/243966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243966/

Comment 6

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

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

https://reviewboard.mozilla.org/r/243966/#review250078

Comment 7

[Patrick Brosset <:pbro>]

Thanks for the review.
The try push shows 1 test is failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76adbb4e1882b841c40c08d54a74e7c2928de46e&group_state=expanded
I fixed this locally (I had regressed the isDisplayed logic a bit, it now returned false if the element was a non-element or didn't have computed styles, which wasn't the case before).
I'll update the commit and land it.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243966/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db12e7724255
Handle computedStyle.display failures for non-elements; r=gl

Comment 10

[Stefan Hindli [:stefan_hindli]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This sounds like it could use at least a Beta approval request. Do we think this is a common enough situation that it might be worth ESR60 consideration as well?

Comment 12

[Patrick Brosset <:pbro>]

I have seen multiple reports recently that seemed quite similar to this. None of them had easy STRs for me to verify that this fix is indeed the right one for them too. But I guess I'm a bit concerned that this bug is impacting a lot of people out there.
So I'll definitely request beta uplift approval. ESR60 would be good too.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1454572
[User impact if declined]: Users of the Inspector panel in DevTools may see missing nodes. This bug shows that in some cases, the entire content of the <body> node may actually be missing. This prevents users from using the inspector at all and getting anything done. This breaks one of the fundamental things that people rely on in DevTools most commonly. This will negatively impact people's trust in the tool, and might force people to move to chrome.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Tested successfully on nightly
[Needs manual test from QE? If yes, steps to reproduce]: None needed
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very small change, basically just adding a try/catch around a line of code, to avoid trying to get computed styles on nodes that don't have them (which is what was changed in bug 1454572).
[String changes made/needed]: None

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Please see my previous comment. This bug has the potential of impacting a lot of users of DevTools out there, and breaks one of the fundamental feature they rely on for pretty much anything.
User impact if declined: See previous comment
Fix Landed on Version: 62.
Risk to taking this patch (and alternatives if risky): This part of the code has not changed between ESR60 and 62. This small patch will apply cleanly there. And as said in my previous comment, it is a minor fix that basically adds a try/catch around a line of code.
String or UUID changes made by this patch: None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

Fixes a pretty severe regression in the inspector. Approved for 61.0b6.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c503530315e

Comment 17

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 62.0a1(20180521100109) and Beta 61.0b6(20180517141400) on Win10 x64, MacOS 10.12 and Ubuntu 16.04

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

devtools inspector fix, approved for 60.1esr

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/3f72cc97499c

Comment 25

[Patrick Brosset <:pbro>]

This is bug is affecting a *lot* of people in release right now.
It's great that we uplifted it to 61 and ESR60. I'm unfamiliar with uplift processes to release. Could we also apply the fix to release 60?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Julien is the Fx60 release owner, redirecting to him.

Comment 27

[Julien Cristau [:jcristau]]

Why does this not have test coverage?

Comment 28

[Patrick Brosset <:pbro>]

(In reply to Julien Cristau [:jcristau] from comment #27)
> Why does this not have test coverage?
Mixture of "quick let's fix this really bad problem as fast as we can now, it's just a few lines of code" and "hmm, how would I even write a test for that" (admitting laziness here).

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Could you please report a bug to write a test? As the regression seems pretty important, would be nice to have a test so that it doesn't come back!

Comment 30

[Julien Cristau [:jcristau]]

Please request uplift to mozilla-release.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1454572
[User impact if declined]: Users of the Inspector panel in DevTools may see missing nodes. This bug shows that in some cases, the entire content of the <body> node may actually be missing. This prevents users from using the inspector at all and getting anything done. This breaks one of the fundamental things that people rely on in DevTools most commonly. This will negatively impact people's trust in the tool, and might force people to move to chrome.
[Is this code covered by automated tests?]: Not yet. I've filed bug 1466533 to add one and I'm investigating a way to create a reduced test case.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: None other than just trying to steps in this bug.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Already landed in ESR60 and working fine there.
[String changes made/needed]: None

Comment 32

[Julien Cristau [:jcristau]]

Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

This is driving 60.0.2.

Comment 33

[Julien Cristau [:jcristau]]

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

Comment 34

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/6f36abb41568 (FIREFOX_ESR_60_0_X_RELBRANCH)

Comment 35

[Gyula Palko[:gpalko]]

Verified as fixed on esr 60.0.2(20180606023959) and release 60.0.2(20180605171542). Tested on Win10 x64, MacOS 10.12 and Ubuntu 16.04

Comment 36

[Patrick Brosset <:pbro>]

Thank you to everyone involved in shipping this all the way to release!

Comment 37

[Garrett W.]

I'm still having this issue sporadically in 64.0b4. It seemed to resolve itself by closing and re-opening the inspector, so I can't reproduce it now to give further info, other than to say my <body> tag was empty when it happened.