Inspector fails to inspect HTML after loading an error page

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: uberj, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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)
(Reporter)

Updated

4 years ago
Summary: Inspector breaks fails to inspect HTML after a page reload → Inspector fails to inspect HTML after a page reload

Comment 1

4 years ago
So only after an error response?
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
Created attachment 8390871 [details]
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
(Reporter)

Comment 4

4 years ago
Note that the error appears immediately after I request the page from the server while it is not running
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
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.
Assignee: nobody → pbrosset
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
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
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.
Duplicate of this bug: 916407
Easiest STR are what pbrosset says in Comment 8.
OS: Linux → All
Hardware: x86_64 → All
Summary: Inspector fails to inspect HTML after a page reload → Inspector fails to inspect HTML after loading an error page
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.
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.
Created attachment 8397040 [details] [diff] [review]
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.
Flags: needinfo?(bgrinstead)
Bug 977043 is refactoring the ProgressListener
Depends on: 977043
(Assignee)

Comment 16

3 years ago
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.
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.
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.
Flags: needinfo?(bgrinstead)
Duplicate of this bug: 945749
Duplicate of this bug: 1130582
Unassigning for now as I'm not working on this bug.
Assignee: pbrosset → nobody
See Also: → bug 909121

Updated

2 years ago
Duplicate of this bug: 1194244
Duplicate of this bug: 1027055
Bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Blocks: 1264624
Duplicate of this bug: 1280581
(Assignee)

Updated

11 months ago
Assignee: nobody → poirot.alex
(Assignee)

Updated

11 months ago
See Also: → bug 1135868
(Assignee)

Updated

11 months ago
See Also: → bug 1277348
(Assignee)

Updated

11 months ago
See Also: → bug 1288846
(Assignee)

Comment 26

11 months ago
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.
(Assignee)

Updated

10 months ago
See Also: → bug 1249119
(Assignee)

Comment 27

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c20bba09b90
(Assignee)

Comment 28

10 months ago
Created attachment 8802190 [details] [diff] [review]
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Depends on: 1277348
(Assignee)

Comment 31

10 months ago
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

10 months ago
mozreview-review
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?
Attachment #8802508 - Flags: review?(jryans)
(Assignee)

Comment 33

10 months ago
mozreview-review-reply
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

10 months ago
mozreview-review
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.
Attachment #8802508 - Flags: review+

Updated

10 months ago
Attachment #8397040 - Attachment is obsolete: true

Comment 35

10 months ago
mozreview-review
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.
Attachment #8802509 - Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 40

10 months ago
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
(Assignee)

Comment 41

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb87dd30ad57
(Assignee)

Comment 42

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d2681b6487
(Assignee)

Comment 43

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a29a1e1342ba
(Assignee)

Comment 44

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed653fe00fe3
(Assignee)

Comment 45

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e478868175d9
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 50

10 months ago
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.
(Assignee)

Comment 51

10 months ago
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.
(Assignee)

Updated

10 months ago
Blocks: 1277348
No longer depends on: 1277348
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

10 months ago
You can see the interdiff between changesets 3 and 4. 4 to 5 is just a rebase.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 65

10 months ago
mozreview-review
Comment on attachment 8804265 [details]
Bug 983386 - Use a common navigateTo helper in inspector tests.

https://reviewboard.mozilla.org/r/88334/#review88050
Attachment #8804265 - Flags: review?(pbrosset) → review+

Comment 66

10 months ago
mozreview-review
Comment on attachment 8804266 [details]
Bug 983386 - Prevent initializing canvas highlighter on loading or destroyed documents.

https://reviewboard.mozilla.org/r/88336/#review88052
Attachment #8804266 - Flags: review?(pbrosset) → review+

Comment 67

10 months ago
Pushed by apoirot@mozilla.com:
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

10 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

10 months ago
No longer blocks: 1264624

Comment 69

10 months ago
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
QA Whiteboard: [testday-20161028]

Comment 70

6 months ago
[bugday-20170301]
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.