|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
23.36 KB, image/png
20.44 KB, patch
|Details | Diff | Splinter Review|
39 bytes, text/x-review-board-request
|Details | Review|
Created attachment 8548966 [details] 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.
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.
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
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)
Oh yeah I am pretty sure this bug only shows up on a navigation or a reload.
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.
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.
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
Created attachment 8559796 [details] 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.
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.
Created attachment 8559826 [details] [diff] [review] 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.
Created attachment 8589309 [details] navigate-quickly-no404.js Same snippet to reproduce errors, not dealing with 404s for now
Created attachment 8589316 [details] [diff] [review] 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.
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
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Created attachment 8592805 [details] [diff] [review] 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).
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+
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.
Created attachment 8593420 [details] 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/
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+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8593420 [details] MozReview Request: bz://1121528/pbrosset
Created attachment 8619126 [details] MozReview Request: Bug 1121528 - Avoid the inspector going blank when quickly navigating; r=bgrins
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
You need to log in before you can comment on or make changes to this bug.