983386 - Inspector fails to inspect HTML after loading an error page

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jacques Uber [:uberj]]

UA: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0


Steps to reproduce:

- Open a page and inspect an element
- Kill the web server that served the original page and refresh the page. You should see an "Unable to connect" message
- Start the web server back up
- Refresh the page and inspect (the inspector will not show HTML in the little DOM pane)

Comment 1

[Dave Camp (:dcamp)]

So only after an error response?

Comment 2

[Jacques Uber [:uberj]]

I think that as long as the server does not respond with any content the error appears. Making the server return a 404 page didn't allow me to reproduce the bug.

Comment 3

[Jacques Uber [:uberj]]

Attachment: errors

Oh! How silly of me to forget...

This is the error that is logged to my terminal running ff when I reproduce the bug

Comment 4

[Jacques Uber [:uberj]]

Note that the error appears immediately after I request the page from the server while it is not running

Comment 5

[Patrick Brosset <:pbro>]

Every time a node is selected in the inspector, we store a uniquely generated css selector for it, with the url of the current page. This means that, when we reload the page, we check if the url hasn't changed, and if it hasn't, we get the previously stored css selector and we try and match a node with it to select it by default.
This way, instead of always having the body node selected by default, you get the last selected node.

I'm surprised this would fail when you stop the server, because the first thing we do is trying to match the css selector and if it doesn't match, we revert to selecting the body node.

Investigation should start here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#207

Comment 6

[Patrick Brosset <:pbro>]

When a page loads, we get its rootNode by calling `walker.getRootNode()`.

In the case where its the same page that reloads but the server is down this time, the rootNode retrieved by getRootNode is, for some reason, a DeadObject wrapper.

In all other cases like reloading the same page with the server running, reloading the same page but with a modified markup, or loading a different page, the rootNode is correctly retrieved, and therefore the rest of the logic here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#207 works well and selects either the last selected node when possible or the body node otherwise.

So something in the way the "Unable to connect" page gets displayed is not playing well with our getRootNode method.

Comment 7

[Patrick Brosset <:pbro>]

I think there's a redirect going on behind the scenes. The URL in the URL bar still is the one for your page, but the page that is displayed really is:
about:neterror?e=connectionFailure&u=some_url_here&c=UTF-8&f=regular&d=some_message_here

Comment 8

[Patrick Brosset <:pbro>]

new STR:

- Open the inspector on any given page
- Load a URL that doesn't point to a server (http://localhost:8001/ for instance)

==> Inspector goes blank

Comment 9

[Brian Grinstead [:bgrins]]

Error page loading is weird and confuses the inspector.  There is a discussion starting here: https://bugzilla.mozilla.org/show_bug.cgi?id=909121#c10.

The issue is that the nsIWebProgressListener is firing the final STATE_STOP before the document is updated with the new page.

Comment 11

[Brian Grinstead [:bgrins]]

Easiest STR are what pbrosset says in Comment 8.

Comment 12

[Patrick Brosset <:pbro>]

Thanks Brian for mentioning this other bug. The discussion there helps focusing on the right part.
One thing I noticed when debugging the error page load process in the ProgressListener is that the flag LOCATION_CHANGE_ERROR_PAGE is never passed in the onStateChange method. That's because it's only passed to the onLocationChange.
So defining this method would allow us to know when we're loading the error page, so hopefully, should help us handle this special case correctly.

Comment 13

[Patrick Brosset <:pbro>]

After more investigation, I can confirm that the situation with the ErrorPage is quite special, and we'll have to have a series of special if/else cases just for it.
Once we know, from the onLocationChange method, that we're going to the ErrorPage, we need to wait the subsequent START/STOP request state changes before firing our "windowchange-stop" event. The hard part to tackle now is to detect which of the STOP state changes we need to wait for, because they are not sent with the IS_WINDOW flag as when loading a standard page.

By just hard coding 3 STOPs, I was able to fix the issue of this bug, but I haven't found yet a way to implement this cleanly.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug983386-markupview-blank-on-errorpage.patch

This is just to support the discussion, not to be checked in.

The bug is indeed with the ProgressListener that gets tricked by error pages.
When loading a normal page, our ProgressListener's onStateChange method gets called as we expect it to.
We basically wait for 2 things:
- IS_DOCUMENT and START, which we interpret as a frame being unloaded
- IS_WINDOW and STOP, which we interpret as a frame has finished loading.
We use these to unload our markup-view and then reload it with the new root node.

In case of the error page, by the time we get the IS_WINDOW + STOP event, the page isn't actually loaded and our onStateChange gets called a few more times with START/STOP state changes. Only these don't have IS_WINDOW.

I haven't found any way to detect which of these STOP states we need to wait for before reloading our markupview on the new root node (I have implemented all onLocationChange(), onProgressChange(), onSecurityChange(), onStateChange() and onStatusChange() methods without getting any meaningful state, status or flag that could help us in this case).
One thing I found that worked in all cases I tested was just waiting for more than 2 of them, but that's of course not a solution we want to keep.

I think there needs to be a fix on the platform side to make the error page loading easier to listen, but I don't know who should be cc'ed on this bug to get this done.

Brian: your input would help since you have some experience with this.

Comment 15

[Brian Grinstead [:bgrins]]

Bug 977043 is refactoring the ProgressListener

Comment 16

[Alexandre Poirot [:ochameau]]

There is an explicit event being fired for the about:neterror redirect.
onLocationChange with flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE.
But that's isn't handy, as this onLocationChange event is being fired a bit after the IS_STOP+IS_WINDOW event we are currently waiting for.
Also something that may be usefull is that, during that event (IS_STOP+IS_WINDOW), request.status & NS_ERROR_CONNECTION_REFUSED. At least we can know something special is happening.

Please try to address that as a followup to bug 977043. I'm in the middle of this code.
I'm trying to be conservative but I have the feeling that existing code "just works", without clear explaination of why we are listening for these particular events. It has most likely be tuned over time to fix some bugs.
I would be really interested to know, if there is any very explicit reason to wait for isDocument+isStart for frameLoad action and isWindow+isStop for frameUnload.
(There is so many other events we can listen for... isWindow for both, isDocument for both, or regular load/unload events, or DOMWindowCreated, or document-element-inserted, or ....)
The TabActor (navigate/will-navigate) and code related to ThreadActor are using isWindow both both events, in addition to also listening for DOMWindowCreated.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

It's not that surprising that these 3 tools (console, debugger, inspector) rely on different state changes. The debugger, which was the first, was only interested at the moment before navigation started and then after the window global was created, hence its reliance on isWindow. The console, which came next, was interested in network events, so that's where the isNetwork came from. The inspector on the other hand deals with a document, so it needs to watch for isDocument state changes mostly.

The still open question is whether we can unify the code handling all those state changes with the minimum number of checks. I wish there was a document or a state diagram that explained all the state transitions, but people were giggling last time I asked for one. Your work in bug 977043 however makes me hopeful that there is a small common subset of state changes that we can use going forward.

Comment 18

[Brian Grinstead [:bgrins]]

Let's (In reply to Alexandre Poirot (:ochameau) from comment #16)
> There is an explicit event being fired for the about:neterror redirect.
> onLocationChange with flags &
> Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE.
> But that's isn't handy, as this onLocationChange event is being fired a bit
> after the IS_STOP+IS_WINDOW event we are currently waiting for.
> Also something that may be usefull is that, during that event
> (IS_STOP+IS_WINDOW), request.status & NS_ERROR_CONNECTION_REFUSED. At least
> we can know something special is happening.
> 
> Please try to address that as a followup to bug 977043. I'm in the middle of
> this code.

I agree - let's wait until after bug 977043.  It sounds like looking for NS_ERROR_CONNECTION_REFUSED might be an easy way to find out that an error is happening.  I believe we would still need some workarounds like in Attachment 8397040 [details] [diff], but it sounds like this might be able to replace the request counting in the patch.

Comment 21

[Patrick Brosset <:pbro>]

Unassigning for now as I'm not working on this bug.

Comment 24

[Patrick Brosset <:pbro>]

Bug triage (filter on CLIMBING SHOES).

Comment 26

[Alexandre Poirot [:ochameau]]

Linking to all bugs where the inspector turns blank and can't be recovered.
The inspector shouldn't break and if it happens to fail in some edge cases it should break so badly so that it doesn't recover when opening a new url.
Also, fixing one bug may fix some others, so I'll revisit them once this one is fixed.

Comment 27

[Alexandre Poirot [:ochameau]]

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

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Ok, it looks like I have a patch for each broken inspector scenario,
with a test for all but the race from bug 1249119.
Time to see if try is green, then polish these patches for review
and figure out in which order they should land.

Comment 29

[Alexandre Poirot [:ochameau]]

Attachment: Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review commit: https://reviewboard.mozilla.org/r/86896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86896/

Comment 30

[Alexandre Poirot [:ochameau]]

Attachment: Bug 983386 - Test the inspector against network error pages.

Review commit: https://reviewboard.mozilla.org/r/86898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86898/

Comment 31

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Note that these patches may depend on bug 1277348 which itself may depend on bug 1249119. At least I'm planning to land them in that order.

So. This patch is the critical fix. What happens is that we were dispatching a navigate event while at that precise time, the document is still the previously loaded page, which will end up with the inspector refering to some dead wrappers. Instead we have to wait a little bit in case of network error and ensure firing the navigate event only once the about:neterror page is loaded.

I haven't tried to introduce tests for this patch as this will be covered by the inspector tests that I'm attaching to this patch.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

https://reviewboard.mozilla.org/r/86896/#review85902

::: devtools/server/actors/webbrowser.js:2499
(Diff revision 1)
>        let newURI = request instanceof Ci.nsIChannel ? request.URI.spec : null;
>        this._tabActor._willNavigate(window, newURI, request);
>      }
>      if (isWindow && isStop) {
> +      // Don't dispatch "navigate" event just yet when there is a redirect to
> +      // about:netrror page.

Nit: about:neterror

::: devtools/server/actors/webbrowser.js:2513
(Diff revision 1)
> +          if (evt.target == window.document) {
> +            handler.removeEventListener("DOMContentLoaded", onLoad, true);
> +            this._tabActor._navigate(window);
> +          }
> +        };
> +        handler.addEventListener("DOMContentLoaded", onLoad, true);

Hmm.  Couldn't we take this path for all page loads, not just network errors?  Or do we lose something that way?

Comment 33

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

https://reviewboard.mozilla.org/r/86896/#review85902

> Hmm.  Couldn't we take this path for all page loads, not just network errors?  Or do we lose something that way?

As commented in the `else` for the common case ("somewhat equivalent of load event. (window.document.readyState == complete)")
the document is already loaded and we should fire the "navigate" event immediately.
isStop && isWindow is somewhat equivalent or really close to the "load" event. I've never been able to prove that's purely equivalent.

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

https://reviewboard.mozilla.org/r/86896/#review85910

Okay, fair enough.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

https://reviewboard.mozilla.org/r/86898/#review86230

::: devtools/client/inspector/test/browser_inspector_navigate_to_errors.js:6
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +// Test that inspector updates when page is navigated.

Maybe update this comment to also mention that you're navigating to a page that doesn't exist. That's the interesting bit of this test.

::: devtools/client/inspector/test/browser_inspector_navigate_to_errors.js:23
(Diff revision 1)
> +  yield navigateTo(inspector, TEST_URL_2);
> +
> +  let documentURI = yield testActor.eval("document.documentURI;");
> +  ok(documentURI.startsWith("about:neterror"), "content is correct.");
> +
> +  let hasPage = yield getNodeFront("#page", inspector);

You're betting here on the fact that about:neterror never gets an element with ID page.
Maybe worth making page more complex and unique.

::: devtools/client/inspector/test/browser_inspector_navigate_to_errors.js:34
(Diff revision 1)
> +  let bundle = Services.strings.createBundle("chrome://global/locale/appstrings.properties");
> +  let domain = TEST_URL_2.match(/^http:\/\/(.*)\/$/)[1];
> +  let errorMsg = bundle.formatStringFromName("connectionFailure",
> +                                             [domain], 1);
> +  is(yield getNodeTextContent("#errorShortDescText", inspector), errorMsg,
> +     "Inpector really inspect the error page");

s/inspect/inspects

::: devtools/client/inspector/test/browser_inspector_open_on_neterror.js:38
(Diff revision 1)
> +
> +  is(yield getNodeTextContent("body", inspector), "content",
> +     "Inspector really inspects the valid url");
> +});
> +
> +function* getNodeTextContent(selector, inspector) {

I would rename this to `getDisplayedNodeTextContent` so that it's easy to understand that it's different from getting the text content from the rawNode itself (perhaps using the test-actor or a contentTask).

And I would move it to head.js so you don't need to repeat it in both of these new tests.

Comment 36

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/1-2/

Comment 37

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/1-2/

Comment 38

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/2-3/

Comment 39

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/2-3/

Comment 40

[Alexandre Poirot [:ochameau]]

It looks like there is some intermittent on the very slow Windows 7 VM:
https://treeherder.mozilla.org/logviewer.html#?job_id=29612199&repo=try
 17:22:38     INFO -  270 INFO TEST-PASS | devtools/client/inspector/test/browser_inspector_open_on_neterror.js | Inspector loaded on the already opened net error -
 17:22:38     INFO -  271 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_open_on_neterror.js | content is really a net error page. -
 17:22:38     INFO -  Stack trace:
17:22:38 INFO - chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_open_on_neterror.js:null:29

Comment 41

[Alexandre Poirot [:ochameau]]

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

Comment 42

[Alexandre Poirot [:ochameau]]

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

Comment 43

[Alexandre Poirot [:ochameau]]

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

Comment 44

[Alexandre Poirot [:ochameau]]

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

Comment 45

[Alexandre Poirot [:ochameau]]

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

Comment 46

[Alexandre Poirot [:ochameau]]

Attachment: Bug 983386 - Use a common navigateTo helper in inspector tests.

Review commit: https://reviewboard.mozilla.org/r/88334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88334/

Comment 47

[Alexandre Poirot [:ochameau]]

Attachment: Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

Review commit: https://reviewboard.mozilla.org/r/88336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88336/

Comment 48

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/3-4/

Comment 49

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/3-4/

Comment 50

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

I moved this changeset from bug 1277348. I'm going to reverse the dependency and land this bug first. I still have issues figuring out the leaks from bug 1277348.
Whereas it looks like I got a green try for this one.

Comment 51

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

This patch is very similar to the one you reviewed in bug 1249119.
But now fixes the CanvasFrameAnonymousContentHelper and the HighlighterActor base class.

Comment 52

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88334/diff/1-2/

Comment 53

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/4-5/

Comment 54

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88336/diff/1-2/

Comment 55

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/4-5/

Comment 56

[Alexandre Poirot [:ochameau]]

You can see the interdiff between changesets 3 and 4. 4 to 5 is just a rebase.

Comment 57

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88334/diff/2-3/

Comment 58

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/5-6/

Comment 59

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88336/diff/2-3/

Comment 60

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/5-6/

Comment 61

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88334/diff/3-4/

Comment 62

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802508 [details]
Bug 983386 - Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86896/diff/6-7/

Comment 63

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88336/diff/3-4/

Comment 64

[Alexandre Poirot [:ochameau]]

Comment on attachment 8802509 [details]
Bug 983386 - Test the inspector against network error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86898/diff/6-7/

Comment 65

[Patrick Brosset <:pbro>]

Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

https://reviewboard.mozilla.org/r/88334/#review88050

Comment 66

[Patrick Brosset <:pbro>]

Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

https://reviewboard.mozilla.org/r/88336/#review88052

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c0e4370ee43
Use a common navigateTo helper in inspector tests. r=pbro
https://hg.mozilla.org/integration/autoland/rev/27d928074eae
Delay "navigate" event when we are redirected to a error page, to only fire once the error page is loaded. r=jryans
https://hg.mozilla.org/integration/autoland/rev/892b7c6952f6
Prevent initializing canvas highlighter on loading or destroyed documents. r=pbro
https://hg.mozilla.org/integration/autoland/rev/0bab744bba82
Test the inspector against network error pages. r=pbro

Comment 68

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1c0e4370ee43
https://hg.mozilla.org/mozilla-central/rev/27d928074eae
https://hg.mozilla.org/mozilla-central/rev/892b7c6952f6
https://hg.mozilla.org/mozilla-central/rev/0bab744bba82

Comment 69

[Saheda Reza [:Antora]]

I have reproduced this bug on firefox nightly according to(2014-03-13)

Fixing bug is verified on Latest Nightly--Build ID:(20161030030204),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0


tested OS-- Windows 10 32bit

Comment 70

[Moneshb7]

[bugday-20170301]