Closed
Bug 1121528
Opened 10 years ago
Closed 10 years ago
Dev tools DOM inspector is sometime empty
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: thomasbelin4, Assigned: pbro)
References
Details
(Whiteboard: [polish-backlog][difficulty=medium])
Attachments
(5 files, 3 obsolete files)
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•10 years ago
|
Component: Untriaged → Developer Tools: Inspector
Comment 1•10 years ago
|
||
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•10 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
Comment 3•10 years ago
|
||
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•10 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•10 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•10 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•10 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
Comment 8•10 years ago
|
||
Needinfo'ing thomas in case he didn't see the comment from pbrosset above.
Flags: needinfo?(thomasbelin4)
Reporter | ||
Comment 9•10 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•10 years ago
|
||
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•10 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•10 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•10 years ago
|
Whiteboard: [devedition-40]
Updated•10 years ago
|
Comment 13•10 years ago
|
||
Same snippet to reproduce errors, not dealing with 404s for now
Comment 14•10 years ago
|
||
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
Comment 15•10 years ago
|
||
We should probably wait for the test rewrite in 1137285 before landing anything here
Depends on: 1137285
Comment 16•10 years ago
|
||
Assigning P1 priority for some devedition-40 bugs.
Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Assignee | ||
Updated•10 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•10 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•10 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.
Comment 19•10 years ago
|
||
(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•10 years ago
|
||
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•10 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•10 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 23•10 years ago
|
||
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 24•10 years ago
|
||
Assignee | ||
Comment 25•10 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.
Assignee | ||
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•10 years ago
|
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8593420 -
Attachment is obsolete: true
Attachment #8619126 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•