Closed Bug 1460223 Opened 6 years ago Closed 6 years ago

Inspector shows empty body for ghacks.net

Categories

(DevTools :: Inspector, defect, P1)

61 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr6060+ verified, firefox60blocking verified, firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 60+ verified
firefox60 blocking verified
firefox61 --- verified
firefox62 --- verified

People

(Reporter: euthanasia_waltz, Assigned: pbro)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
Has Regression Range: --- → yes
Keywords: regression
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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
I found the root cause. Will push a fix soon.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

https://reviewboard.mozilla.org/r/243966/#review250078
Attachment #8975750 - Flags: review?(gl) → review+
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.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db12e7724255
Handle computedStyle.display failures for non-elements; r=gl
https://hg.mozilla.org/mozilla-central/rev/db12e7724255
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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?
Blocks: 1454572
Flags: qe-verify+
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
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
Attachment #8975750 - Flags: approval-mozilla-beta?
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
Attachment #8975750 - Flags: approval-mozilla-esr60?
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.
Attachment #8975750 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on Nightly 62.0a1(20180521100109) and Beta 61.0b6(20180517141400) on Win10 x64, MacOS 10.12 and Ubuntu 16.04
Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

devtools inspector fix, approved for 60.1esr
Attachment #8975750 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
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?
Flags: needinfo?(ryanvm)
Julien is the Fx60 release owner, redirecting to him.
Flags: needinfo?(ryanvm) → needinfo?(jcristau)
Why does this not have test coverage?
Flags: needinfo?(pbrosset)
(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).
Flags: needinfo?(pbrosset)
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!
Blocks: 1466533
Flags: needinfo?(jcristau)
Please request uplift to mozilla-release.
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(pbrosset)
Attachment #8975750 - Flags: approval-mozilla-release?
Comment on attachment 8975750 [details]
Bug 1460223 - Handle computedStyle.display failures for non-elements;

This is driving 60.0.2.
Attachment #8975750 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-esr60/rev/6f36abb41568 (FIREFOX_ESR_60_0_X_RELBRANCH)
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Thank you to everyone involved in shipping this all the way to release!
Product: Firefox → DevTools
Flags: in-qa-testsuite+
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.
You need to log in before you can comment on or make changes to this bug.