1121528 - Dev tools DOM inspector is sometime empty

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[thomasbelin4]

Attachment: Screen Shot 2015-01-14 at 12.20.59.png

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.

Comment 1

[Eddy Bruel [:ejpbruel]]

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.

Comment 2

[thomasbelin4]

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

Comment 3

[Eddy Bruel [:ejpbruel]]

Patrick, could the stack trace in comment #1 explain the behavior we're seeing here?

Comment 4

[Patrick Brosset <:pbro>]

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?

Comment 5

[thomasbelin4]

Oh yeah I am pretty sure this bug only shows up on a navigation or a reload.

Comment 6

[Patrick Brosset <:pbro>]

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.

Comment 7

[Patrick Brosset <:pbro>]

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.

Comment 8

[Eddy Bruel [:ejpbruel]]

Needinfo'ing thomas in case he didn't see the comment from pbrosset above.

Comment 9

[thomasbelin4]

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

Comment 10

[Patrick Brosset <:pbro>]

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

Comment 11

[Patrick Brosset <:pbro>]

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.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1121528-blank-inspector.patch

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.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: navigate-quickly-no404.js

Same snippet to reproduce errors, not dealing with 404s for now

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: blank-inspector.patch

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.

Comment 15

[Brian Grinstead [:bgrins]]

We should probably wait for the test rewrite in 1137285 before landing anything here

Comment 16

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug1121528-blank-inspector-on-quick-navigation.patch

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

Comment 18

[Patrick Brosset <:pbro>]

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.

Comment 19

[Brian Grinstead [:bgrins]]

(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 20

[Brian Grinstead [:bgrins]]

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

Comment 21

[Patrick Brosset <:pbro>]

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.

Comment 22

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1121528/pbrosset

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

Comment 23

[Brian Grinstead [:bgrins]]

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?

Comment 24

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0da58c8e096

Comment 25

[Patrick Brosset <:pbro>]

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.

Comment 26

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/be78b7a11cdd

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1121528 - Avoid the inspector going blank when quickly navigating; r=bgrins