1036324 - If you select an element while the page is reloading and the Inspector is open, the markup view disappears and does not reappear even after the page has finished loading

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Elmo Allén]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

1. Open a page, any page but for testing preferably something with slow-loading scripts.
2. Select an element, any element, to the DOM Inspector. DOM code view should show normally.
3. Reload page.
4. Select again an element (the same element or any other) before the page has finished reloading, but when the DOM structure should be already loaded (e.g. some scripts are still loading).


Actual results:

When you select the element again, the code view of the DOM Inspector disappears, and does not reappear even if you wait for the page to finish loading. If you close the Developer Tools and reopen it, the code view shows normally, even if the page is still loading. If you wait for the page to finish loading and _do_not_select_ any element while loading, the code reappears.

The element path view behaves as expected: the element is selected, and you can use it to navigate to other elements. Also the CSS views (Rules, Computed, Fonts, Box Model) show the correct values for the element. You can also select new elements by using the mouse right-click on the page, and the element path view updates as expected, as well as the CSS view. Only the code view stays hidden all this time.

If you change to other Developer tabs, e.g. JavaScript Debugger, it doesn't help. Only closing and reopening the Developer Tools completely makes the code in DOM Inspector reappear.


Expected results:

The code should be shown in the DOM Inspector immediately after the DOM is ready, even if the page is still loading some content or scripts.

Comment 1

[:Gijs (he/him)]

Elmo, can you link to an example page where your see this? This has been reported before, but your steps seem more concrete than those in bug 1014459, if only slightly... It's been difficult to reproduce reliably.

Brian, does this help with investigating the issue?

Comment 2

[Elmo Allén]

OK, here's an example with which I can make the bug to reproduce. It makes an AJAX query to keep the page in loading stage (demonstrated by the running circle icon in the tab). The key here is only to get the page to be in the loading stage after the DOM has been readied, so any script waiting to be loaded should do the same.

http://elmoallen.name/firefox_inspector_bug/

Comment 3

[Elmo Allén]

Second example, quite a bit more simple: http://elmoallen.name/firefox_inspector_bug/test2.html

This one uses an image which it request from the localhost, which doesn't usually respond (you'll have to stop your HTTP server if you have any) and the browser keeps the page in loading stage until the image timeouts. During the timeout you'll have to open the Inspector and hit refresh. And before the image again timeouts, select an element.

Comment 4

[Elmo Allén]

Sorry, a minor mistake: you don't have to be fast enough to hit reload before the first loading of the page finishes. Just open the Inspector and hit refresh, and during that refresh, you'll just have to be fast enough to select any element (e.g. right-clicking and hitting Q on any part of the page).

Comment 5

[:Gijs (he/him)]

Thanks!

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Elmo Allén from comment #2)
> OK, here's an example with which I can make the bug to reproduce. It makes
> an AJAX query to keep the page in loading stage (demonstrated by the running
> circle icon in the tab). The key here is only to get the page to be in the
> loading stage after the DOM has been readied, so any script waiting to be
> loaded should do the same.
> 
> http://elmoallen.name/firefox_inspector_bug/

Thanks - I can confirm with http://elmoallen.name/firefox_inspector_bug/, by reloading twice then right click -> inspect element while waiting for the 10 second timeout.  This will be a huge help in narrowing the problem down.

(In reply to Elmo Allén from comment #3)
> Second example, quite a bit more simple:
> http://elmoallen.name/firefox_inspector_bug/test2.html

I can't reproduce with no localhost server runnine.  The image seems to fail immediately, either that or I'm not doing it quick enough.

Comment 7

[Elmo Allén]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Elmo Allén from comment #3)
> > Second example, quite a bit more simple:
> > http://elmoallen.name/firefox_inspector_bug/test2.html
> 
> I can't reproduce with no localhost server runnine.  The image seems to fail
> immediately, either that or I'm not doing it quick enough.

You can try again now. I changed the image to an address http://example.com:8080/, which seems to put my browser into not finishing the reload.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Elmo Allén from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > (In reply to Elmo Allén from comment #3)
> > > Second example, quite a bit more simple:
> > > http://elmoallen.name/firefox_inspector_bug/test2.html
> > 
> > I can't reproduce with no localhost server runnine.  The image seems to fail
> > immediately, either that or I'm not doing it quick enough.
> 
> You can try again now. I changed the image to an address
> http://example.com:8080/, which seems to put my browser into not finishing
> the reload.

Yes, that does the trick.

1) Load http://elmoallen.name/firefox_inspector_bug/test2.html
2) Stop loading and open inspector
3) Reload the page
4) Select an element while it is still spinning.  Notice that the breadcrumbs and rule view update, but the markup view doesn't have any content yet.
5) Wait for a while until it stops trying to load the image (takes a while ~30 seconds).

Now the markup view is messed up if you navigate to new pages

Comment 9

[Brian Grinstead [:bgrins]]

Elmo, what is the PHP source of the ajax script at elmoallen.name/firefox_inspector_bug/ajaxload.php?  I think I have a fix, but I'm working on adding a test case that reproduces the issue.

Comment 10

[Elmo Allén]

(Back from holiday.) Brian, here:

<?php

sleep(8);
echo "<em>AJAX ready.</em>";

?>

So basically stopping for a moment and then outputting something.

Comment 11

[Brian Grinstead [:bgrins]]

Bug 777674 is cleaning up some of the walker bugs (calls to _reparentWalker are throwing errors when following STR from comment 8).  It's proven hard so far to create a mochitest that reproduces the problem in the same way (I'm thinking that the apache server is setting some headers that the sjs files in the test harness are not)

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: markup-reload-WIP.patch

This fixes the problem following STR from comment 8 by simply not throwing when trying to set an invalid node in the walker, but I still haven't been able to get a test to fail in the same way.

Comment 13

[Brian Grinstead [:bgrins]]

The good news is that I can't reproduce the completely broken markup view problem anymore (probably since Bug 777674).  The bad news is that we still show a completely blank markup view while waiting for that image to load.  We need to fix that.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> The good news is that I can't reproduce the completely broken markup view
> problem anymore

I spoke too soon, just reproduced it :(

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: markup-load-test-case.patch

Finally, I've been able to find a way to make a failing test case for this.  It starts up an HTTP server and loads an image resource that takes a moment to display.  Then it reloads and selects an element from the page via the context menu (but before the page load event fires).  This causes the markup view to get messed up and the test to fail.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Created attachment 8584848 [details] [diff] [review]
> markup-load-test-case.patch
> 
> Finally, I've been able to find a way to make a failing test case for this. 
> It starts up an HTTP server and loads an image resource that takes a moment
> to display.  Then it reloads and selects an element from the page via the
> context menu (but before the page load event fires).  This causes the markup
> view to get messed up and the test to fail.
I'm currently investigating this issue, thanks for the test case, that helps a lot.
I'm doing this while working on bug 1121528, so I might fix the issue there if I find the root cause.
So far, I think the issue lies with MarkupView._onNewSelection which is called twice in a row, without waiting for the first call to resolve, and since this method doesn't deal at all with concurrent calls, that leads to problems.

Comment 17

[Patrick Brosset <:pbro>]

That's incorrect, the onNewSelection mechanism seems to work fine. In fact everything seems to work fine, except the view is empty.
I now believe that the importNode function is the problem. Namely, we never go into this condition:

    if (aNode === this.walker.rootNode) {
      container = new RootContainer(this, aNode);
      this._elt.appendChild(container.elt);
      this._rootNode = aNode;
    }

So, we never append the root node (#document) to the markup-view, and so the other imported nodes are being appended off DOM.

Comment 18

[Patrick Brosset <:pbro>]

When running the test with devtools.debugger.log=true, the #document node that's missing has the ID server1.conn0.domnode55.
By looking at the logs, I can see that this actor is never transmitted to the client.
The client has a mechanism whereby it creates a "standin" node until it receives the serialized form to replace it with. The problem is it never receives the form.
See http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2825

Comment 19

[Patrick Brosset <:pbro>]

\o/ I found the root cause!
After reloading the page, the actor for the new root-node for the new page was being created as server1.conn0.domnode53, but it later was getting orphaned somehow by the walker, and all orphan nodes get cleaned up at some stage: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2450
So, because it wasn't here, when the child <html> (documentElement) node was getting sent to the client, it would ask for the parent, and create a standin node for it (server1.conn0.domnode55), which never got serialized to the client of course (server1.conn0.domnode5 was serialized, not this new one).
Hence, no root node, nothing in the markup-view.

Now, what actually was making the real root node an orphan was the long-deprecated but still used WalkerFront.frontForRawNode method, here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#3103
It's unfortunately still being used when the browser contextual-menu is used, as seen here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#522
(the else of the if(isRemote) uses selection.setNode, which calls frontForRawNode).
I filed bug 1154645 to refactor this part.
That's why the test case only fails when the contextual menu is used.

frontForRawNode made the test case fail because at some stage, it gets the list of parents for the given node (which it needs to do, to make sure all parents leading up to the root are known) by doing:
let extras = walkerActor.parents(nodeActor);
It then goes up to the upmost parent and checks if it's the same as the currently known root node, if it's not, it makes it an orphan, hence it gets cleaned up as said before.

Why is the upmost parent not the currently know root node? Because for some reasons I don't know, we should have done:
let extras = walkerActor.parents(nodeActor, {sameDocument: true});
instead, to avoid the walker going back up to the browser XUL (if you look carefully while the test run, you should see that the breadcrumbs suddenly jumps and shows all sorts of XUL nodes, that's the whole browser structure).

So adding {sameDocument: true} fixes the issue.

Of course it's never that simple, and it won't work correctly if the node you right-click on is inside an iframe. Not sure how to fix this one though.
The thing with this bug is that if we:
- ship e10s by default soon and
- start populating the markup-view on DOMContentLoaded rather than load,
it should go away, right?
So I'm not so inclined in coming up with a complex solution to fix it.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: Avoid blank inspector when using 'inspec element' menu while reloading

Comment 21

[:Gijs (he/him)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #19)
> \o/ I found the root cause!
> After reloading the page, the actor for the new root-node for the new page
> was being created as server1.conn0.domnode53, but it later was getting
> orphaned somehow by the walker, and all orphan nodes get cleaned up at some
> stage:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> inspector.js#2450
> So, because it wasn't here, when the child <html> (documentElement) node was
> getting sent to the client, it would ask for the parent, and create a
> standin node for it (server1.conn0.domnode55), which never got serialized to
> the client of course (server1.conn0.domnode5 was serialized, not this new
> one).
> Hence, no root node, nothing in the markup-view.
> 
> Now, what actually was making the real root node an orphan was the
> long-deprecated but still used WalkerFront.frontForRawNode method, here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> inspector.js#3103
> It's unfortunately still being used when the browser contextual-menu is
> used, as seen here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#522
> (the else of the if(isRemote) uses selection.setNode, which calls
> frontForRawNode).
> I filed bug 1154645 to refactor this part.
> That's why the test case only fails when the contextual menu is used.
> 
> frontForRawNode made the test case fail because at some stage, it gets the
> list of parents for the given node (which it needs to do, to make sure all
> parents leading up to the root are known) by doing:
> let extras = walkerActor.parents(nodeActor);
> It then goes up to the upmost parent and checks if it's the same as the
> currently known root node, if it's not, it makes it an orphan, hence it gets
> cleaned up as said before.
> 
> Why is the upmost parent not the currently know root node? Because for some
> reasons I don't know, we should have done:

Nice!

> let extras = walkerActor.parents(nodeActor, {sameDocument: true});
> instead, to avoid the walker going back up to the browser XUL (if you look
> carefully while the test run, you should see that the breadcrumbs suddenly
> jumps and shows all sorts of XUL nodes, that's the whole browser structure).
> 
> So adding {sameDocument: true} fixes the issue.
> 
> Of course it's never that simple, and it won't work correctly if the node
> you right-click on is inside an iframe. Not sure how to fix this one though.
> The thing with this bug is that if we:
> - ship e10s by default soon and
> - start populating the markup-view on DOMContentLoaded rather than load,
> it should go away, right?
> So I'm not so inclined in coming up with a complex solution to fix it.

I don't know how hard it is to teach the walkerActor this distinction, but essentially the difference between the browser XUL and a parent document would be that the docshell type changes. See in particular the relevant bits on nsIDocShellTreeItem, like:

73 	/*
74 	Returns the root DocShellTreeItem of the same type.  This is a convience 
75 	equivalent to getting the parent of the same type and its parent until 
76 	there isn't a parent.
77 	*/
78 	readonly attribute nsIDocShellTreeItem sameTypeRootTreeItem;

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShellTreeItem.idl#53

I don't know how soon we intend to make the switch to DOMContentLoaded, so I don't know if it's workable to "add work" here... YMMV.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #21)
> I don't know how soon we intend to make the switch to DOMContentLoaded, so I
> don't know if it's workable to "add work" here... YMMV.

We are using a webProgressListener for this: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#2299.  I looked into it briefly and I couldn't find a good way to simulate DOMContentLoaded - using isWindow && isStop seems to not fire until all resources on the page are loaded.  If you have any ideas about how to get ahold of an earlier event, I filed Bug 1151909.

Comment 23

[Brian Grinstead [:bgrins]]

Firstly, Patrick, great work tracking this down!

I'm worried about losing the 'inspect element' functionality within iframes, especially for 40.  So I'd like to see if we can either:

1) Implement something like what Gijs suggests in Comment 21 - which could be switched on based on isLocal() on the Walker (so that it didn't break Browser Toolbox functionality).
2) Do the refactor in Bug 1154645 so that local and non-local use the same code paths

I think we will want to get to 2 eventually, and it doesn't seem like that should be too hard to have a frame script loaded in both contexts.  But if it is, something like 1 could be a workaround.

Comment 24

[Brian Grinstead [:bgrins]]

I will put together a test to see if I can get option 1 working

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: walker-docshells.patch

Using Gijs' approach in Comment 21.   Something like this seems like it could work

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> Created attachment 8593023 [details] [diff] [review]
> walker-docshells.patch
> 
> Using Gijs' approach in Comment 21.   Something like this seems like it
> could work

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=311e33233fad.  Patrick and I discussed that we would need to check backwards-compatibility support to ensure the new option isn't causing any problems on older servers.  I haven't done this yet.  Marking as depending on Bug 1121528 since I think we'll want to rebase on top of it.

Comment 27

[Brian Grinstead [:bgrins]]

I guess this one is "easy" at this point since we've already done all the hard work

Comment 28

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> Patrick and I discussed that we would need to check backwards-compatibility
> support to ensure the new option isn't causing any problems on older
> servers.  I haven't done this yet.
I just tested this and it works beautifully, protocol.js doesn't seem to validate the options passed to the method. So we can add the sameTypeRootTreeItem option to the method. When the front-end talks to an older server, this extra option will just be ignored.

> Marking as depending on Bug 1121528
> since I think we'll want to rebase on top of it.
I just landed bug 1121528 to fx-team, so I'll rebase the patch, fold in the test-case patch too, and test locally and on try.
Thanks Brian.

Comment 29

[Patrick Brosset <:pbro>]

Attachment: bug1036324-blank-inspector-on-navigate.patch

Rebased/folded patch

Comment 30

[Patrick Brosset <:pbro>]

Brian, you wrote both the test case and the docshell loop thing in this patch, so it makes more sense if the bug is assigned to you I think.
That try push is green, so I guess it's ready for final review and landing.

I've got one question about a change you made in markup-view.js:

> @@ -2140,17 +2140,17 @@ MarkupElementContainer.prototype = Herit
>          data.data.string().then(str => {
>            let res = {data: str, size: data.size};
>            // Resolving the data promise and, to always keep tooltipData.data
>            // as a promise, create a new one that resolves immediately
>            def.resolve(res);
>            this.tooltipData.data = promise.resolve(res);
>          });
>        }, () => {
> -        this.tooltipData.data = promise.reject();
> +        this.tooltipData.data = promise.resolve({});
>        });
>      }
>    },
>  
>    /**
>     * Executed by MarkupView._isImagePreviewTarget which is itself called when the
>     * mouse hovers over a target in the markup-view.
>     * Checks if the target is indeed something we want to have an image tooltip
> @@ -2160,19 +2160,21 @@ MarkupElementContainer.prototype = Herit
>     * to decide if/when to show the tooltip
>     */
>    isImagePreviewTarget: function(target, tooltip) {
>      if (!this.tooltipData || this.tooltipData.target !== target) {
>        return promise.reject();
>      }
>  
>      return this.tooltipData.data.then(({data, size}) => {
> -      tooltip.setImageContent(data, size);
> -    }, () => {
> -      tooltip.setBrokenImageContent();
> +      if (data && size) {
> +        tooltip.setImageContent(data, size);
> +      } else {
> +        tooltip.setBrokenImageContent();
> +      }
>      });
>    },

Why not keep the rejected promise in _prepareImagePreview and rejection handler in isImagePreviewTarget?

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Why not keep the rejected promise in _prepareImagePreview and rejection
> handler in isImagePreviewTarget?

I tried this, but I get the following error immediately upon rejecting in the test case (regardless of whether there is a rejection handler in isImagePreviewTarget):

41 INFO TEST-UNEXPECTED-FAIL | browser/devtools/markupview/test/browser_markupview_load_01.js | A promise chain failed to handle a rejection:  - undefined
Stack trace:
    JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 162
    JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 675
    JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/markupview/markup-view.js :: MarkupElementContainer.prototype<._prepareImagePreview/< :: line 2148
    etc...

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: inspector-reload.patch

Thanks for folding and rebasing this, that helped a lot.

Interestingly, walker.parents() is only used in the frontForRawNode function which is only used in a local connection.  So in that case, backwards compat as far as changing params isn't an issue here (although I still think that adding a new Option() doesn't break compat in general, I'm not positive).

As far as the promise in the markup view (Comment 30 / 31), I've spent a bit trying to work that out but haven't figured out a way around it.  I'd be happy to look more into it if you think it's important.

Here's an updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f604152b13cd

Comment 33

[Patrick Brosset <:pbro>]

Comment on attachment 8594224 [details] [diff] [review]
inspector-reload.patch

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

::: browser/devtools/markupview/test/head.js
@@ +654,5 @@
>      return yield waitForMultipleChildrenUpdates(inspector);
>    }
>  }
> +
> +function createTestHTTPServer() {

This helper function needs a more self-explanatory name and a bit of jsdoc comment.
It doesn't only create a test server but also set it up to handle slow.gif requests and make that response slow.

Comment 34

[Brian Grinstead [:bgrins]]

Attachment: inspector-reload-r=pbrosset.patch

Updated test server organization and comments.  Moving the slow.gif handler into the specific test.  If we need to reuse certain request types at some point we could move it into the shared.js helper.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bf2ad9d35e6

Comment 35

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #34)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bf2ad9d35e6

There's some orange on windows builds but no indication that it's related to this work

Comment 36

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/00574d3ed81d

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00574d3ed81d