Dev tools DOM inspector is sometime empty

RESOLVED FIXED in Firefox 40

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: thomasbelin4, Assigned: pbro)

Tracking

unspecified
Firefox 40
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150112004004

Steps to reproduce:

Cannot find a clear way to reproduce the bug. 
Just opened the dev tools, then navigating on the app I am inspecting and, eventually, the DOM inspector will ends up being totally empty.


Actual results:

The DOM inspector is empty.
This used to happen to me sometime on Firefox 34 (stable channel) and still happens to me on FirefoxDeveloperEdition 36.0a2.

Here is my user agent : "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0"


Expected results:

The DOM inspector should show me the DOM tree of the current page.
(Reporter)

Updated

4 years ago
Component: Untriaged → Developer Tools: Inspector
Thanks for reporting this. We can't really act upon this bug without having steps to reproduce, but it will be useful to keep track of this issue. If more people start reporting this bug, we will eventually have to take a closer look.
(Reporter)

Comment 2

4 years ago
Looking at the browser console, I found this trace. As it occurs in the inspector.js file, I guess it might be related. Hope it helps.

TypeError: can't access dead object
Stack trace:
WalkerActor<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1727:9
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1004:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1485:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
Patrick, could the stack trace in comment #1 explain the behavior we're seeing here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pbrosset)
(Assignee)

Comment 4

4 years ago
It could definitely explain the blank markup-view.
It probably is a timing issue whereby the request to walker.querySelector gets handled after the page has navigated or something, and therefore the node it tries to execute it on is a DeadObject.
At least that's my guess right now, it's hard to confirm without steps to reproduce.

Thomas, was there a page navigation or redirect when that occurred?
Flags: needinfo?(pbrosset) → needinfo?(thomasbelin4)
(Reporter)

Comment 5

4 years ago
Oh yeah I am pretty sure this bug only shows up on a navigation or a reload.
Flags: needinfo?(thomasbelin4)
(Assignee)

Comment 6

4 years ago
Thanks. So, walker.querySelector isn't the only actor method to use a node, in fact, most of the walker's methods do this, so potentially all methods are subject to this if they happen to be called during/just before a refresh/navigation.
2 things to investigate:
- make the actor less prone to these timing issues
- try and make the front-end carry on initializing even though one of the requests failed. In this particular case, I'm almost sure the querySelector is called to retrieve the default inspector selection, so if that fails, it shouldn't be a big deal, the markup view should still be able to load even though the default node selection is incorrect.
Now the hard part is actually finding a way to reproduce this bug.
(Assignee)

Comment 7

4 years ago
Changing a few flags, as I think this doesn't only happen on OSX and I can't think of anything in 37 that would have fixed this, so it must still be here.
OS: Mac OS X → All
Hardware: x86 → All
Version: 36 Branch → unspecified
Needinfo'ing thomas in case he didn't see the comment from pbrosset above.
Flags: needinfo?(thomasbelin4)
(Reporter)

Comment 9

4 years ago
Ok so having paid a little more attention to when the bug occurs, it is definitely on a page reload/navigation.  

I hope it was the info you needed because I am not sure I see any other question from pbrosset in the thread and I (almost :)) already answered this question here https://bugzilla.mozilla.org/show_bug.cgi?id=1121528#c5
Flags: needinfo?(thomasbelin4)
(Assignee)

Comment 10

4 years ago
Posted file navigate-quickly.js
I can't reproduce this bug easily by hand, so I ended up creating a scratchpad script that causes the issue.
This goal of this script is not to be beautiful, so it's not :) but it does load a new URL in the tab in an interval, very quickly, and after some time, it stops the interval, waits for the inspector to settle, and checks its content. If it's empty, it stops there. Otherwise it starts again.
I was able very easily to get the inspector empty a few times with this, although I didn't get the same error as reported in this bug. I suspect we may uncover a lot of different timing issues with this kind of test.
(Assignee)

Comment 11

4 years ago
I have fixed the first few obvious errors this script has shown me (I may upload a patch later, but for now, it's very messy, just early returns here and there to avoid errors).
Once done, I still had a few issues, but I realized they were all due to 404 responses. If I put only real URLs in my script, then the inspector is never blank again. But 404 responses seem to have special treatment.
(Assignee)

Comment 12

4 years ago
This is the patch I'm using right now to avoid the issue. It's basically making the walker's methods defensive by checking if the provided node is a deadWrapper first.
(Assignee)

Updated

4 years ago
Whiteboard: [devedition-40]
See Also: → 1036324
Same snippet to reproduce errors, not dealing with 404s for now
Posted patch blank-inspector.patch (obsolete) — Splinter Review
Rebased and a slightly different approach to isDead() by making a separate isNodeDead function that will handle a null node also.  Additionally, not making document/documentElement nullable and instead just returning the proper element if a dead node is passed in.

This still isn't passing my test case in Bug 1036324, but seems like a good start.
Attachment #8559826 - Attachment is obsolete: true
We should probably wait for the test rewrite in 1137285 before landing anything here
Depends on: 1137285
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
(Assignee)

Updated

4 years ago
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
(Assignee)

Updated

4 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 17

4 years ago
Rebased, and added some more checks. With this patch in, the navigate-quickly script doesn't seem to be able to break the inspector (it does sometimes remain blank, but that's when Firefox itself becomes unresponsive for a while, never managing to fully load a URL).
Attachment #8589316 - Attachment is obsolete: true
Attachment #8592805 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 18

4 years ago
Not sure what test I could add for this, it's mostly a big list of early returns from various functions for hard to reach cases.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> Not sure what test I could add for this, it's mostly a big list of early
> returns from various functions for hard to reach cases.

I have two ideas:
1) We could add a server test that exercises the full walker API, passing in null to each and expecting the appropriate return value.
2) We could use something like the test case in Bug 1036324 -> createTestHTTPServer where we set up URLs that take various times to respond and then do something like the navigate-quickly scratchpad test.  Only we should try to take away any randomness..  It'd be a great regression test if we could get something like that set up but I'm not sure how much the element of randomness has to do with with it.
Comment on attachment 8592805 [details] [diff] [review]
bug1121528-blank-inspector-on-quick-navigation.patch

Review of attachment 8592805 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 19.  The changes look fine - I'd be OK with a simple server test to ensure that the walker functions return the expected results when a null element is passed in.  If it's not too much work, it'd be great to also add a regression test that simulates this problem, but if it's not possible to come up with a reproducible test I'd be fine with the server side test only
Attachment #8592805 - Flags: feedback?(bgrinstead) → feedback+
(Assignee)

Comment 21

4 years ago
I spent most of this morning trying to come up with a complex test case that would make the inspector blank. I tried a number of things, mostly combining making the protocol requests slow, and making the http server slow. I wasn't able to get an empty markup-view.
And in fact, I've tried running the navigate-quickly script on dev-edition just now, for a long while, and couldn't get it to have an empty markup-view either!
So at this stage, this patch is really more of a cleanup patch that avoids lots of exceptions in the logs, and errors in the walker.
So I will now be working on the simpler server-side test you suggested.
(Assignee)

Comment 22

4 years ago
/r/7127 - Bug 1121528 - Avoid the inspector going blank when quickly navigating; r=bgrins

Pull down this commit:

hg pull -r e7cff0254b9cd634b434d0508469fe102b6148c8 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8593420 - Flags: review?(bgrinstead)
Comment on attachment 8593420 [details]
MozReview Request: bz://1121528/pbrosset

https://reviewboard.mozilla.org/r/7125/#review5917

Nice!

::: browser/devtools/styleinspector/rule-view.js
(Diff revision 1)
> +      return "";

I'm assuming the return is in place to catch the rejection? If this is the case I think the following is equivalent: 

}).catch(console.error);

::: toolkit/devtools/server/actors/inspector.js
(Diff revision 1)
> -    if (this.rawNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
> +    if (!this.rawNode ||

Should this be !isNodeDead(this) to catch the dead wrapper case?
Attachment #8593420 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 25

4 years ago
https://reviewboard.mozilla.org/r/7125/#review5951

::: browser/devtools/styleinspector/rule-view.js
(Diff revision 1)
> +      return "";

I originally  added `return ""` to make sure `getOriginalSourceStrings` would always return a value so that the caller wouldn't have to deal with `getOriginalLocation` rejections. Using catch wouldn't make this work.
But upon further investigation, I found that returning the `""` string wasn't a good idea, because when the call succeeds, it doesn't return a string, but an object containing strings (`sourceStrings`).
And the caller (`updateSourceLink`) already has a rejection handler, so no need for one here, we should just let the error bubble up to the caller.

::: toolkit/devtools/server/actors/inspector.js
(Diff revision 1)
> -    if (this.rawNode.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE ||
> +    if (!this.rawNode ||

Yes you're right, will change this.
https://hg.mozilla.org/mozilla-central/rev/be78b7a11cdd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
See Also: → 939012
(Assignee)

Comment 28

4 years ago
Attachment #8593420 - Attachment is obsolete: true
Attachment #8619126 - Flags: review+
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.