917448 - WalkerActor.querySelector: can't access dead object error

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

I attempted to come up with a reduced test case here, but I did not have much luck.  In any case, here's what I was doing:

1. Go to http://www.massrelevance.com
2. Open the Inspector
3. Refresh the page
4. The Inspector panel dies

Here's the stack trace from the Browser Console:

Message: "can't access dead object"

WalkerActor<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1327
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:906
DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1018
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:255
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:72

Comment 1

[Paul Rouget [:paul]]

I see 2 iframes in this page. Might be related.

Also: bug 848731, bug 754612 & bug 758422.

Comment 2

[Patrick Brosset <:pbro>]

I've seen this happen often indeed, not necessarily on pages that did contain iframes, although I can't be sure.
I'll take this bug and try to see if I can come with a more reduced test case.

Comment 3

[Patrick Brosset <:pbro>]

- Go to http://jsbin.com/welcome/1/edit
- select an element in the preview panel on the right side
- reload the page
- inspector goes blank

Comment 4

[Patrick Brosset <:pbro>]

Also reproduced using jryans' URL : http://www.massrelevance.com/ 
- go to http://www.massrelevance.com/
- open the inspector (not even need to select an element)
- reload the page
- inspector goes blank

And in fact, in the previous test case, no need to select an element either.
It seems that as soon as an iframe is here, reloading the page makes the inspector goes blank with this error

  Message: TypeError: can't access dead object
  Stack:
    WalkerActor<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1327
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:906
DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1020
LDT_send/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:255
makeInfallible/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:72

Comment 5

[Brian Grinstead [:bgrins]]

Reading the description, I thought it would be related to Bug 909121.  However, it is also happening in Aurora, so it seems to be a different issue.  

I was also able to reproduce using both of these steps on Nightly and Aurora:

- Go to http://jsbin.com/welcome/1/edit
- Open Inspector 
- Reload page

- Go to http://www.massrelevance.com/
- Open Inspector 
- Reload page

Comment 6

[Brian Grinstead [:bgrins]]

I was talking with pbrosset about this, and wanted to add a little more information that we found after debugging a bit.  The order of the load (nsIWebProgressListener) events seems to be coming through differently on the broken pages.  Listening to the onFrameLoad and onFrameUnload events, here is what I see on a good page with iframes: 

On page: http://fiddle.jshell.net/bgrins/bgJGb/show/
console.log: UNLOAD http://fiddle.jshell.net/bgrins/bgJGb/show/
console.log: UNLOAD about:blank
console.log: UNLOAD about:blank
console.log: LOAD http://bgrins.github.io/
console.log: LOAD http://bgrins.github.io/
console.log: LOAD http://fiddle.jshell.net/bgrins/bgJGb/show/

Notice that everything unloads, then everything loads.  Here is what happens on a bad page.  Notice that load events on about:blank are firing right away:

On page: http://jsbin.com/welcome/1/edit
console.log: UNLOAD http://jsbin.com/welcome/1/edit
console.log: UNLOAD about:blank
console.log: LOAD about:blank
console.log: UNLOAD about:blank
console.log: LOAD about:blank
console.log: UNLOAD about:blank
console.log: LOAD about:blank
console.log: UNLOAD about:blank
console.error:
Message: TypeError: can't access dead object
Stack:
WalkerActor<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1327
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:906
DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1018
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:255
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:72
console.error:
unknownError
console.log: LOAD http://run.jsbin.com/runner
console.log: UNLOAD about:blank
console.log: LOAD about:blank
console.log: UNLOAD http://run.jsbin.com/runner
console.log: LOAD http://run.jsbin.com/runner
console.log: LOAD http://jsbin.com/welcome/1/edit

I believe that the weird order of events is causing `this.layoutHelpers.getFrameElement(window)` in onFrameLoad to return null, even though it isn't the top level window.  This causes it to fire a newRoot mutation with the wrong document.  Changing the condition to `window === window.top` seeems to fix the problem.  Coincidentally, we just had an email to the list: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/l3mKbG0yRn4 about not using window.top.  In this case, frameElement is null even for the about:blank pages.  Still haven't narrowed down *why* it fails on some pages with frames, but not others.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: windowtop.patch

Not saying we should use this, but just to show some code to go along with Comment 6.  This allows inspector to reload http://jsbin.com/welcome/1/edit and  http://www.massrelevance.com/.

Comment 8

[Patrick Brosset <:pbro>]

Couldn't it be that it fails when iframes are created through javascript rather than just being in the HTML source? It looks like it's the case on both the massrelevance and jsbin sites but not on the jsfiddle site.

Comment 9

[Patrick Brosset <:pbro>]

I'm still mostly in the dark here, progressing one little step at a time. So far, what I've nailed it down to is in LayoutHelpers.jsm/getFrameElement(win):

On the jsbin site, for one of the frames being loaded (with about:blank), the flow through that function is as follows:

  getFrameElement: function LH_getFrameElement(win) {
    if (this.isTopLevelWindow(win)) { // returns false, so the frame is NOT a top level, which is expected
      return null;
    }

    let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIWebNavigation)
                   .QueryInterface(Ci.nsIDocShell);

    if (docShell.isBrowserOrApp) { // returns false, so we skip to the else
      let parentDocShell = docShell.getSameTypeParentIgnoreBrowserAndAppBoundaries();
      let parentDoc = parentDocShell.contentViewer.DOMDocument;
      let allIframes = parentDoc.querySelectorAll("iframe");
      for (let f of allIframes) {
        if (f.contentWindow === win) {
          return f;
        }
      }
      return null;
    } else {
      return win.frameElement; // returns TRUE!!! knowing that if it does, it normally means it's a top level window!
    }
  },

So that particular iframe is in a state where we know it's not top level, but still frameElement returns null
(https://developer.mozilla.org/en/docs/Web/API/window.frameElement)

Comment 10

[Patrick Brosset <:pbro>]

sorry, I meant // returns NULL!!! in the last step

Comment 11

[Patrick Brosset <:pbro>]

Discussing with Paul Rouget, one change we want to do in this function is get rid of the ` if (docShell.isBrowserOrApp)` condition and always use the docShell in all cases.

Doing this, we still have the issue for the problematic iframe on the jsbin website.
This time, what's happening is that none of the child iframes retrieved with the querSelectorAll match the current window, so the `for of` loop reaches the end and the function returns null.

Comment 12

[Paul Rouget [:paul]]

(In reply to Patrick Brosset from comment #11)
> Discussing with Paul Rouget, one change we want to do in this function is
> get rid of the ` if (docShell.isBrowserOrApp)` condition and always use the
> docShell in all cases.
> 
> Doing this, we still have the issue for the problematic iframe on the jsbin
> website.
> This time, what's happening is that none of the child iframes retrieved with
> the querSelectorAll match the current window, so the `for of` loop reaches
> the end and the function returns null.

When exactly do we do that (getting the frameElement)? What is the status of the inner document (DOM ready?).

Comment 13

[Patrick Brosset <:pbro>]

This is called by the inspector (server-side) in the onFrameLoad function (events.on(this.progressListener, "windowchange-stop", this.onFrameLoad);)

Comment 14

[Patrick Brosset <:pbro>]

It seems to me that this is what happens:

- the inspector listens to "windowchange-stop" on the progressListener, 
- therefore the onFrameLoad is called when sub iframes are loading
- in the case with the jsbin website, these iframes are created dynamically through javascript and are not present in the HTML code of the page when it loads.
- still in the onFrameLoad function, we try to get the frame for this iframe using docShell.getSameTypeParentIgnoreBrowserAndAppBoundaries().contentViewer.DOMDocument.querySelectorAll("iframe"), but this returns 0 nodes, leading the inspector to think that this dynamically created iframes is root!

Comment 15

[Patrick Brosset <:pbro>]

Also, still on the jsbin site, the onFrameLoad callback is never executed for the root window (http://jsbin.com/)

That means that ProgressListener.onStateChange in the Inspector.js file never fires the windowchange-stop event for the root window.

Documentation at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference suggests that the when a document request completes, there should finally be a STATE_IS_WINDOW flag combined with a STATE_STOP, however, this never happens for http://jsbin.com/

Investigating a bit further, I can see that on that website, there are 2 requests that never end, probably causing the root document from never completing its load.

What ultimately causes the problem is that in the inspector/onFrameLoad function, as soon as you have one frame that is loaded and that has no parent, and if there hasn't been any rootDoc before, then this one is considered root.
So on jsbin, it turns out one of the frames somehow has no parent, that's one problem, and the other problem is that the actual root never finishes loading, so that frame gets to be the root, causing the rest of the problems.

Comment 16

[Patrick Brosset <:pbro>]

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference also describes STATE_IS_NETWORK which seems to work a lot better.
I'm attaching a patch now that resolves the issue, but seems to introduce an issue in the computed css view (should be easy to fix).

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug917448-cant-access-dead-object.patch

This is not the final patch. There's still this css computed-view error that we need to get rid of. But it seems to do the trick for the main issue (although I have to admit I'm not entirely sure why).

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 807913 [details] [diff] [review]
bug917448-cant-access-dead-object.patch

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

It fixes these failures, and prevents the style errors we were seeing earlier, so that is good quite good.  I do wish that we could generate a failing case that we could add to our tests so that this doesn't regress in the future, and so that we could come up with variations on whatever exactly is triggering it to test out other edge case failures.  It could be worth taking a deeper look at the jsbin code to try and come up with a smaller test case, since it is open source: https://github.com/remy/jsbin/ and https://github.com/remy/jsbin/blob/master/docs/running-your-own-jsbin.md.

::: toolkit/devtools/server/actors/inspector.js
@@ +741,3 @@
>        events.emit(this, "windowchange-start", progress.DOMWindow);
>      }
> +    if (isNetwork && (flags & Ci.nsIWebProgressListener.STATE_STOP)) {

I don't fully know the implications of using isNetwork here.  There is a difference according to https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWebProgressListener#State_Type_Flags, but I'm just not completely sure of it.

> STATE_IS_NETWORK 
> Unlike STATE_IS_WINDOW, this flag is only set when activity within the nsIWebProgress instance being observed starts or stops.

> STATE_IS_WINDOW
> The first has STATE_IS_REQUEST and STATE_IS_DOCUMENT set, and the second has the STATE_IS_WINDOW flag set (and possibly the STATE_IS_NETWORK flag set as well -- see above for a description of when the STATE_IS_NETWORK flag may be set)

The takeaway is that there are times when STATE_IS_WINDOW is set, but STATE_IS_NETWORK is not set (presumably the situation triggering this bug).  However, can we be sure that we will not miss important requests using this method?  For instance, what if the src of the iframe is a data url, would the STATE_IS_NETWORK still fire?

  <iframe src="data:text/html;charset=utf-8,%3Chtml%3E%3Cbody%3Efoo%3C/body%3E%3C/html%3E" />

Comment 19

[Patrick Brosset <:pbro>]

Attachment: iframe.html

Yay!! I went through a whole lot of the jsbin code and finally was able to find what in there was causing the inspector to go blank.

See the HTML attached.

What's happening is that upon page load, an iframe is created, then immediately deleted.

Comment 20

[Patrick Brosset <:pbro>]

Even simpler test case:

<!DOCTYPE html>
<html lang="en">
<head>
	<meta charset="UTF-8">
	<title>iframe</title>
</head>
<body>
  <div id="live"></div>
	<script type="text/javascript">
      var iframe = document.createElement('iframe');
      document.body.appendChild(iframe);
	</script>
</body>
</html>

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug917448-cant-access-dead-object.patch

Here is a new version of the same patch posted earlier, but with a bonus test case.

/!\ Note that in the test case, I manipulate an iframe inside the test HTML document to reproduce the issue, however, had I done the same from the test JS file instead, it would have failed with the same "dead object" exception. So the fix isn't perfect, but perhaps sufficient for this bug/for the time being.

Comment 22

[Patrick Brosset <:pbro>]

Comment 20 is wrong, sorry about that, here's the revised version:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>iframe</title>
</head>
<body>
  <script type="text/javascript">
    var iframe = document.createElement('iframe');
    document.body.appendChild(iframe);
    iframe.parentNode.removeChild(iframe);
  </script>
</body>
</html>

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #21)

> /!\ Note that in the test case, I manipulate an iframe inside the test HTML
> document to reproduce the issue, however, had I done the same from the test
> JS file instead, it would have failed with the same "dead object" exception.
> So the fix isn't perfect, but perhaps sufficient for this bug/for the time
> being.

This case is still throwing an error if the iframe creation/deletion happens on load or on DOMContentLoaded.  This is probably the same reason why it fails when running directly inside of the mochitest.  It would probably be worth adding both test cases to our test file (the one you already have, plus the one on load and the one on DOMContentLoaded).  Here is the case that fails with the patch applied:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>iframe</title>
</head>
<body>
  <div id="live"></div>
  <script type="text/javascript">
      window.onload = function() {
        var iframe = document.createElement('iframe');
        document.body.appendChild(iframe);
        iframe.parentNode.removeChild(iframe);
      }
  </script>
</body>
</html>

Another failing case: 

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>iframe</title>
</head>
<body>
  <div id="live"></div>
  <script type="text/javascript">
      document.addEventListener("DOMContentLoaded", function() {
        var iframe = document.createElement('iframe');
        document.body.appendChild(iframe);
        iframe.parentNode.removeChild(iframe);
      })
  </script>
</body>
</html>

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: deadobject.patch

Check isTopLevelWindow in addition to the reference to frame to prevent errors when frame is removed during different events.

Update test to check DOMContentLoaded, window.onload, inline, and check from mochitest.  Also, move the test to inspector folder, and remove bug number from the test names.

Patrick, we may want to revert changes to LayoutHelpers.getFrameElement, since they shouldn't be needed with this fix.  I left it as-is because I wasn't sure if the changes were an improvement regardless of this bug, but feel free to revert if you think that would be best and reupload.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: deadobject-1.patch

Same as Comment 24, but adds missing test files

Comment 26

[Patrick Brosset <:pbro>]

Thank you Brian for the fix.
Here is a TRY build of it: https://tbpl.mozilla.org/?tree=Try&rev=5e6b388cdfc0

Comment 27

[Patrick Brosset <:pbro>]

Ok, let's TRY again ... https://tbpl.mozilla.org/?tree=Try&rev=32a6aa81a214

Comment 28

[Patrick Brosset <:pbro>]

And one more ... https://tbpl.mozilla.org/?tree=Try&rev=4c882c009c2e

Comment 29

[Patrick Brosset <:pbro>]

Attachment: bug917448-cant-access-dead-object.patch

Thanks to Brian, here is the latest patch for this bug.
On top of trying to get the frame element for each frame that finishes loading, we now also check if the frame is a top level element.

This to avoid the original issue where one of the frames (in jsbin.com) finished loading before the top level one, and was deleted just after, which was tricking the inspector into thinking it was the top level one, hence the "dead node" exceptions.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e03622902f74

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e03622902f74