Open Bug 1277348 Opened 8 years ago Updated 2 years ago

Inspector never loads and then can't close toolbox once it's been opened (somehow doc.write() related?)

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(Not tracked)

People

(Reporter: miketaylr, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(4 files)

test-case.html
249 bytes, text/html
Details
test-case-2.html
144 bytes, text/html
Details
Bug 1277348 - Fix support of document.open against the inspector.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1277348 - Use a common navigateTo helper in inspector tests.
58 bytes, text/x-review-board-request
pbro
: review+
Details 
STR:

1) Navigate to https://miketaylr.com/code/datalist.html
2) Open devtools
3) Click inspector panel, notice that it's empty
4) Attempt to close devtools (shortcuts or X)

Expected:
Can see stuff, close devtools

Actual:
Busted. Only way to close devtools is to close the tab. Chrome devtools works.

I'm guessing something about the following bit of code I added to a linked script is making devtools unhappy:

(at the bottom of https://miketaylr.com/scripts/jquery.datalist.js)

document.body.onload = function(){
  document.write("<img style='position:fixed;width:100%;height:100%;top:0;left:0;right:0' src='https://miketaylr.com/scripts/giphy.gif'>");
}
(just confirmed that if I add `document.close()` after `document.write()` everything works as expected. Is there no way for devtools to recover from this on its own?
Seems like this could be related to Bug 1151909.  Moving over to Inspector for triage
Component: Developer Tools → Developer Tools: Inspector
See Also: → 1151909
For me the console does work, it's just the inspector that's busted on that page (of course also I can't close the toolbox)
Summary: All devtools panels broken, can't close once open (somehow doc.write() related?) → Inspector never loads and then can't close toolbox once it's been opened (somehow doc.write() related?)
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P1
See Also: → 1096031
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attached file test-case.html — 
Test case in case the URL provided expires.

Interesting fact:
If you open the inspector on another site before, and then navigate to the test case page, everything works fine. The markup loads in the inspector, and the toolbox can be closed.

The problem only happens if you first load the page, and then open the inspector.
The problem comes from the InspectorActor.getWalker method, which never returns if document.close() is missing.
Here's the method: http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/devtools/server/actors/inspector.js#2608-2636
In case the page is loading (which is the case here because document.close() hasn't been called), then we start listening for DOMContentLoaded to resolve the returned promise. That never happens and so the inspector stays empty because it never finishes to initalize.
The inspector initialization happens in toolbox.js here: http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/devtools/client/framework/toolbox.js#1910-1933

getWalker being the first thing we do in this case. initInspector is used in other panels too, like the console, so I wouldn't be surprised if other panels were affected by this.
While the document stream is being written to (as soon as document.open or document.write has been called), then the document.readyState is set to loading, and it will not change until document.close has been called.

So the InspectorActor thinks the document is loading, but it's not, and so it just waits for a DOMContentLoaded event that never comes.
Like, I can add a setTimeout in the code to resolve the promise returned by getWalker after some time, and that works out fine.
We need a way to detect that the document has really loaded but is being written to.

Note that, as I said in comment 5, if you open the devtools on another page first, and then navigate to the test case page, it works fine. That's because we've already gotten a WalkerActor at that point. So the buggy code isn't being execute again on that page.
Attached file test-case-2.html — 
Here's a different manifestation of the same problem. Use these STRs with this new test case:

- open the URL in a new tab
- quickly open the inspector (you have 5 seconds)
- notice that the inspector is correctly loaded, with "content 1" being the only content in the page
- wait until the timeout expires
- notice that the page goes back in loading state, and the inspector goes empty

Now, if you switch to the console and execute 'document.close()' then things are going to go back to normal.
I can't find a solution to the problem right now. I've been looking for a way to detect whether the DOM had loaded once before, even if document.write has been called since. We don't really care that the document has readyState=loading again, we can construct a DocumentWalker now, and anything that gets written with document.write after that will come in as mutations anyway.

So, if readyState=loading *but* somehow we know that DOMContentLoaded was fired before, then don't wait on that event, and just resolve the promise returned by getWalker.

I'm going to downgrade this to P2 based on these assumptions, hoping that they're not too far from the reality:
- document.write/open/close isn't used much,
- document.write without a document.close is not advised,
- this only happens if you start writing after the load event has fired.
Priority: P1 → P2
See Also: → 81980
Alex, quick question about the TabActor:

would checking that a given window exists in tabActor._progressListener._knownWindowIDs mean that that window is ready?

I guess I'd need to define what "ready" means here. When the inspector opens for the very first time, it tries to instantiate a WalkerActor. But it wants to make sure the DOM is ready first. So if document.readyState is not "loading", it goes ahead and instantiates it, otherwise it waits for the DOMContentLoaded event.
However as noted in previous comments, this event won't be fired in that case.

So, why does the WalkerActor needs to wait until the DOM is ready? I think that's because one of the first things it does is start a MutationObserver on DOM nodes.

So, I'm wondering if the fact that the progresslistener knows about a window already (i.e. tabActor._windowReady has been called for this window) means that its DOM is ready to be used.
Flags: needinfo?(poirot.alex)
If it is in knownWindowIDs, it means that DOMWindowCreated has been fired.
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2998-3010
newGlobal event from the Debugger API is also fired at the same time.
And nsGlobalWindow::SetNewDocument is done. Apart from that I'm not sure of anything.

For the inspector usecase I would have more thought about document-element-inserted.
This is one of the very first event fired when a document loads. It is fired when we are done parsing the DOM tree, it is fired before any JS from the page is evaluated but the document object is fully usable.

But you may want to use the nsIWebProgressListener from webbrowser.js.
This is used by Firefox UI (to control the stop/reload button) and doesn't break when document.write is used.
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js?q=file%3Awebbrowser.js&redirect_type=single#2509-2540

I think the condition "isDocument && isStop" is the earliest event that would match inspector needs and may be equivalent to document-element-inserted. But we do not fire any event in that case, nor do we store windows in any Map/Set.
Flags: needinfo?(poirot.alex)
Stealing. This will most likely conflict with bug 983386, which I already started looking at.
I would like to fix both bugs in the same timeframe.
Assignee: pbrosset → poirot.alex
See Also: → 983386
As bug 1249119, I'll wait to figure out bug 983386 before r? as these patches may collide between each others...
Comment on attachment 8802498 [details]
Bug 1277348 - Use a common navigateTo helper in inspector tests.

These patches may depend on bug 1249119.
In this first patch, I'm just factoring out navigateTo, cleaning things up where it is already used to wait for the most important events, especially inspector-updated which I think may be redundant with the other one, as it should be dispatched after the others are. But it may help seeing which steps fail?

I'm about to use this help for the various test against broken inspectors.
Comment on attachment 8800771 [details]
Bug 1277348 - Fix support of document.open against the inspector.

As you already suggested, a way to fix this is to allow the inspector to start inspecting the DOM immediately and not wait for load nor DOMContentLoaded.
Note that's introduce a significant difference. It will allow the inspector to immediately show up some DOM when you open it on a loading page.
Whereas if the inspector is already opened and you refresh the page, it will still wait for the "navigate" event to fire, which relates to the load event!
I think that's something we could/should change and let the inspector update as the page loads, but that's another story and it looks much more challenging to do.
Comment on attachment 8800771 [details]
Bug 1277348 - Fix support of document.open against the inspector.

Also note that the documentElement fix addresses things for this document.open() story as well as bug 1249119 with the startup race. Regarding bug 1249119 it just prevents exception from being logged, it doesn't fix it. Whereas here it is important to prevent this documentElement from throwing.
Depends on: 1249119
Comment on attachment 8802498 [details]
Bug 1277348 - Use a common navigateTo helper in inspector tests.

https://reviewboard.mozilla.org/r/86886/#review86222
Attachment #8802498 - Flags: review?(pbrosset) → review+
Comment on attachment 8800771 [details]
Bug 1277348 - Fix support of document.open against the inspector.

https://reviewboard.mozilla.org/r/85634/#review86226

Just a couple of nits.
This makes the situation a lot better already, thanks.
The second set of STRs I gave in comment 8 still don't work though, but that can be addressed separately.

::: devtools/client/inspector/test/browser_inspector_open_with_document_open.js:21
(Diff revision 3)
> +
> +add_task(function* () {
> +  // Test opening the inspector on such document
> +  yield addTab(TEST_URL);
> +  let { inspector } = yield openInspector();
> +  ok(true, "Inspector loaded on the already opened document");

Is this assertion enough? Don't we need to test that the inspector is not empty? Or maybe it's enough that we yield on openInspector. Just want a confirmation.

::: devtools/server/actors/inspector.js:1092
(Diff revision 3)
>     *        to use the root document.
>     */
>    documentElement: function (node) {
> -    let elt = isNodeDead(node)
> -              ? this.rootDoc.documentElement
> -              : nodeDocument(node.rawNode).documentElement;
> +    let elt;
> +    if (isNodeDead(node)) {
> +      // On loading document, the documentElement may not be available yet,

nit: "On loading" should be "In a loading".
Attachment #8800771 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset <:pbro> from comment #24)
> Comment on attachment 8800771 [details]
> Bug 1277348 - Fix support of document.open against the inspector.
> 
> https://reviewboard.mozilla.org/r/85634/#review86226
> 
> Just a couple of nits.
> This makes the situation a lot better already, thanks.
> The second set of STRs I gave in comment 8 still don't work though, but that
> can be addressed separately.

I'll file a followup. I would like to merge all these inspectors tweaks from my queue!

> :::
> devtools/client/inspector/test/browser_inspector_open_with_document_open.js:
> 21
> (Diff revision 3)
> > +
> > +add_task(function* () {
> > +  // Test opening the inspector on such document
> > +  yield addTab(TEST_URL);
> > +  let { inspector } = yield openInspector();
> > +  ok(true, "Inspector loaded on the already opened document");
> 
> Is this assertion enough? Don't we need to test that the inspector is not
> empty? Or maybe it's enough that we yield on openInspector. Just want a
> confirmation.

It is enough to say "the inspector loads", which wasn't the case.
But yes it may not be enough to ensure it is not empty (not sure about that),
and it doesn't ensure it is inspecting the right document.
I'll add a call to selectNode.
Depends on: 1311694
Comment on attachment 8800771 [details]
Bug 1277348 - Fix support of document.open against the inspector.

https://reviewboard.mozilla.org/r/85634/#review86278

::: devtools/client/inspector/test/browser_inspector_open_with_document_open.js:27
(Diff revision 3)
> +
> +  // But also test navigating to it
> +  yield navigateTo(inspector, "data:text/html,foo");
> +
> +  yield navigateTo(inspector, TEST_URL);
> +  ok(true, "Inspector loaded on the navigated document");

Actually, this navigate back to the document calling document.open, and highlighs the missing scenario you referenced in comment 8.
As I don't have any easy way to address that, I removed this and will keep that for bug 1311694.

So to be clear. This patch is going to fix the inspector when we are opening the inspector on such already loaded document, but it won't fix the scenario when the inspector is already opened and you are navigating to such document -or- calling document.open/write after the inspector is fully set.
It looks like there is some intermittent on slow Window 7 VM:
https://treeherder.mozilla.org/logviewer.html#?job_id=29610993&repo=try#L12716
17:07:45 WARNING - TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_open_with_document_open.js | leaked 1 docShell(s) until shutdown 

And unfortunately that's an intermittent leak only on windows 7 vm debug :x
No longer blocks: 983386
Depends on: 983386
Hum. That's "interesting".
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c684051c878&selectedJob=30313137
On e10s-dt4 for VM7 debug, the leak is reported in the logs, but treeherder is still green.

So, I still have the leak issue on windows debug.
Product: Firefox → DevTools
Assignee: poirot.alex → nobody
Status: ASSIGNED → NEW

It looks like bug 1489308 did not fix this. I don't know what inspector is doing here, exactly, and why it's failing; document.open should not look like a navigation anymore!

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: