1259743 - Inline sources in iframe stop being displayed after few reloads

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jarda Snajdr [:jsnajdr]]

Attachment: Node.js script needed to reproduce

Steps to reproduce:
1. Run the Node script in the attachment - it serves two files, test.html and test-sub.html. You'll need to npm install express first.
2. Open http://localhost:5000/test.html in Firefox.
3. Open the debugger - it will show the test-sub.html file as a source, with the inline script.
4. Hit the reload button a few times.

Actual result:
After a few reloads, the test-sub.html script will disappear and debugger will start saying "This page has no sources".

Expected results:
The inline script in the iframe document is always displayed.

If the script is in the top-level document, and not in the iframe, then the buggy behavior doesn't happen. I set the fn() function as an onclick handler of a DIV to avoid GC.

Comment 1

[Eddy Bruel [:ejpbruel]]

The most likely cause for inline scripts disappearing between reloads is that they are garbage collected.
However, as you pointed out, you've deliberately set the function as an onclick handler of a div to prevent just that. So the question becomes: is the inline script indeed being garbage collected? And if so, why is it not being retained?

This is something we should look into, but its not a show stopper. Assigning P2 to this.

Comment 2

[Jarda Snajdr [:jsnajdr]]

An observation I didn't mention before: this happens for me only in e10s. When e10s is disabled, the reload always succeeds.

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: Inline sources in iframe stop being displayed after few reloads

I found why this happens and I also created a fix, not sure if it's the right one.

Why are the sources lost after a few reloads? Because notifications about new sources are received only when the DebuggerProgressListener.onWindowCreated event handler [1] is properly called on a newly created top-level window. And it isn't, because the event handler on docShell.chromeEventHandler's DOMWindowCreated event (added at [2]) is incorrectly removed.

Why? The handlers are added in the DebuggerProgressListener.watch method [3], when a new root docShell is created (at [4]). They are removed when that docShell is destroyed. When loading a page containing iframe, two root docshells are created:

1. for the top-level document. It remains alive for the whole lifetime of the actor, and it only navigates from one page to another during reloads.
2. for the iframe, a root docShell is created for "about:blank" URL and then navigated to the iframe's real URL. This docShell is destroyed a while after the page is unloaded. This docShell gets created only in e10s. In non-e10s mode, no notification about a new docshell arrives - only a notification about a new window. That's why this bug happens only in e10s.

The most important thing: those two docShells share the same chromeEventHandler! It's the same object.

So, during reload, the sequence of events is this:

1. will-navigate event, signaling the reload. The debugger frontend clears the source list on this event.
2. new top-level window is created, the onWindowCreated handler is called (sometimes)
3. new docshell is created for iframe, handlers are attached to its chromeEventHandler
4. the docshell from previous iframe is destroyed, handlers are removed (for everyone, including the top-level docshell!)
5. Page is reloaded again, going through steps 1 and 2 again.
6. In step 2 above, there are no handlers to notice the DOMWindowCreated event.

The onWindowCreated handler calls the actor._windowReady method, which resets the server-side source list [5]. When this reset doesn't happen, then later notifications about new sources are ignored, because the actor thinks it already has these sources [6]. The frontend isn't notified about them and its source list remains empty.

How did I fix it?
- if the window.location is "about:blank", don't start watching the new docShell.
- if we start watching a docShell, add it to a WeakSet and don't call unwatch on it unless it's there. This is necessary, because the docShell doesn't have the "about:blank" URL any more when being unwatch()-ed, so we need some way to prevent removing handlers that were never added.

The storage actor already ignores "about:blank" windows, too [7] - I got inspiration here.

I'm not sure at all whether this patch is correct - I'm in a completely new territory and I didn't know what docShell is before I started investigating this. Maybe the check in _isRootDocShell is incorrect? If it didn't detect the "about:blank" docShell as root, it would solve the bug, too.

The attached patch contains the fix, and also a lot of well-placed console.log() statements that you can use to replay the story told above.

Asking Eddy for feedback.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#2245-2263
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#2209
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#2201-2218
[4] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1159
[5] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1718
[6] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#195
[7] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/storage.js#2261

Comment 4

[Jarda Snajdr [:jsnajdr]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

isRootDocShell should only return true for *one* docshell per document load.
It's not 100% clear, but it looks like your are seeing cases where it condiders two docshells as being root? Is that really the case?
If yes, you are saying that they all have chromeEventHandler set.
So it means that they aren't DOMElement.
Could you try to dump them to see what they are?


I don't think that the about:blank check is good.
When you load an iframe it always first starts loading about:blank. So it doesn't mean much. The same happen with tabs. When you open one, it always starts with loading about:blank.

Comment 6

[Jarda Snajdr [:jsnajdr]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> It's not 100% clear, but it looks like your are seeing cases where it
> condiders two docshells as being root? Is that really the case?
> If yes, you are saying that they all have chromeEventHandler set.
> So it means that they aren't DOMElement.
> Could you try to dump them to see what they are?

Yes, two docshells are considered root by the isRootDocShell method: the top-level one and the iframe one, too.

Both have a chromeEventHandler that is the same object. removeEventListener from docShell2 removes a listener added on docShell1.

This chromeEventHandler is a WindowRoot object (has WindowRootPrototype proto).

Comment 7

[Alexandre Poirot [:ochameau]]

Weird. Could you confirm it reproduces the same thing with this data URI:
data:text/html,<iframe src="data:text/html,<script>function fn() { console.log('hello'); }</script><div onclick='fn()'>hello</div>"/>

I got the same breakage after a few load, but wanted to confirm it doesn't change the code behavior.

Comment 8

[Jarda Snajdr [:jsnajdr]]

Attachment: Inline sources in iframe stop being displayed after few reloads

Comment 9

[Jarda Snajdr [:jsnajdr]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> Weird. Could you confirm it reproduces the same thing with this data URI:

Yes, it behaves exactly the same way with the data URIs.

I attached a new version of the patch that removes the check for "about:blank", and instead reimplements the isRootDocShell check to test the window's parent. It now doesn't look at the chromeEventHandler at all. Works perfectly for me.

Also, when deciding whether to call progressListener.unwatch in _notifyDocShellDestroy, I no longer test with isRootDocShell, because that's unreliable: a docShell can be non-root when created, and be root when destroyed. This happens to the iframe docShell, probably because its parent window was destroyed before it, leaving it orphaned. I check the watchedDocShells WeakSet instead.

> isRootDocShell should only return true for *one* docshell per document load.

In this case, I don't understand what the code at [1] is doing and when is it called. If the original top-level document is destroyed, it's docShell has been root, wasn't it? Because it was top-level. So how can another document be top-level, too? Maybe when the original root docShell was destroyed, one of its child docShells became a root?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1257-1277

Comment 10

[Alexandre Poirot [:ochameau]]

I'll take a serious look at your findings on Monday.

(In reply to Jarda Snajdr [:jsnajdr] from comment #9)
> > isRootDocShell should only return true for *one* docshell per document load.
> 
> In this case, I don't understand what the code at [1] is doing and when is
> it called. If the original top-level document is destroyed, it's docShell
> has been root, wasn't it? Because it was top-level. So how can another
> document be top-level, too? Maybe when the original root docShell was
> destroyed, one of its child docShells became a root?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> webbrowser.js#1257-1277

It used to happen on Firefox OS. New document were spawn, like activities that are top level frames, not rooted to any other (chromeEventHandler is null for them if I remember correctly).

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8744214 [details] [diff] [review]
Inline sources in iframe stop being displayed after few reloads

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

Ok. So I applied the patch and checked various cases.
This isRootDocShell is used only in two cases:
* b2g/firefox-os, with special iframes created in the child process.
When opening an activity or calling window.open, a new root docshell
is created in the child process.
* browser toolbox, we watch for new top level windows in order to be able to track their sub frames.

It think it looks good overall.
Could you submit a new patch with comment addressed and if you have a test against that, that would be even better!

::: devtools/server/actors/webbrowser.js
@@ +1203,5 @@
>      // Non-root docshell have a chromeEventHandler that is either
>      // xul:iframe, xul:browser or html:iframe.
> +    let win = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindow);
> +    return (win === win.parent);

DOMWindow.parent is not correct.
nsIDocShell.parent is better.

I would suggest using this and updating the comment to something like this:
// Should report as root docshell:
//  - New top level window's docshells, when using ChromeActor against a
// process. It allows tracking iframes of the newly opened windows
// like Browser console or new browser windows.
//  - MozActivities or window.open frames on B2G, where a new root docshell
// is spawn in the child process of the app.
return !docShell.parent;

@@ +1260,5 @@
>                       }]
>                     });
>  
> +    // Stop watching this docshell (the method will check if we started watching
> +    // it before).

the method -> `watch()`

@@ +2233,5 @@
>  
>    watch: function(docShell) {
> +    let docShellWindow = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindow);
> +    this._watchedDocShells.add(docShellWindow);

Could you use stop docShell instead of windows?
Or does WeakSet throws when passing docshells?

Comment 12

[Jarda Snajdr [:jsnajdr]]

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

Comment 13

[Jarda Snajdr [:jsnajdr]]

Attachment: Inline sources in iframe stop being displayed after few reloads

Comment 14

[Jarda Snajdr [:jsnajdr]]

Attachment: make devtools/server/actors/webbrowser.js eslint-clean

Comment 15

[Jarda Snajdr [:jsnajdr]]

Attachment: remove the mustNotifyServiceWorkerRegistrationChanged check as it's dead code

Comment 16

[Jarda Snajdr [:jsnajdr]]

Finished the patch to its final form:
- changed implementation of _isRootDocShell to use !docShell.parent, with comment suggested by Alex
- watched docShells are added to a WeakSet, and unwatched only when present in the WeakSet
- added a mochitest that reloads the page several times and checks the NEW_SOURCE events. The test times out (waiting for the event that will never come) when ran on an unpatched Firefox.

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> Could you use stop docShell instead of windows?
> Or does WeakSet throws when passing docshells?

Yes, WeakSet rejects docShells because they are not wrappercached. I asked about this on IRC the other day and was advised by :bz to use the window as a key, as they are in 1:1 correspondence. And referred to bug 655297 for more details.

As a follow-up, I fixed all eslint errors in the webbrowser.js file. It also involved removing a piece of dead code that referred to undefined variable "swm". I extracted this change as a separate patch and will ask :ejpbruel for review of this particular thing.

Submitted a try run and it's 100% green.

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8745888 [details] [diff] [review]
make devtools/server/actors/webbrowser.js eslint-clean

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

Thanks for this cleanup, hopefully there isn't too many people with patches against this file.

::: devtools/server/actors/webbrowser.js
@@ +357,5 @@
> +
> +  actor = new BrowserTabActor(this._connection, browser);
> +  this._actorByBrowser.set(browser, actor);
> +  this._checkListening();
> +  return promise.resolve(actor);

Is it rejected by eslint? I found the original pattern easy to read/understand.

@@ +362,4 @@
>  };
>  
>  BrowserTabList.prototype.getTab = function({ outerWindowID, tabId }) {
> +  if (typeof outerWindowID == "number") {

Out of curiosity, does eslint rejects `typeof()`?
This form is less common, but I actually prefer it as it makes it less magical.

@@ +715,5 @@
> + *   will-navigate,
> + *   window-destroyed,
> + *   changed-toplevel-document,
> + *   window-ready,
> + *   navigate

May be prefix this with - or * to make it look more like a list

@@ +815,5 @@
>     * Getter for the tab's doc shell.
>     */
>    get docShell() {
> +    throw Error(
> +      "The docShell getter should be implemented by a subclass of TabActor");

I think it is more correct to do:
  throw new Error()
rather than
  throw Error()

@@ +955,5 @@
>    update: function() {
>      return promise.resolve(this);
>    },
>  
> +  form: function() {

While you are at it, you may even switch to es6 syntax:
  form() {

For this and all others cases like that.

@@ +2170,3 @@
>      }
> +
> +    return this.connect();

So eslint isn't able to figure out that if there is a `return` in all if/elseif/else patterns, the function actually return an explicit value in any case?

Comment 18

[Alexandre Poirot [:ochameau]]

I'm wondering if you could remove webbrowser.js from eslintignore, by doing same thing than webconsole's panel.js:
http://mxr.mozilla.org/mozilla-central/source/.eslintignore#106
So that we don't regress your work...

Comment 19

[Jarda Snajdr [:jsnajdr]]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> ::: devtools/server/actors/webbrowser.js
> @@ +357,5 @@
> > +
> > +  actor = new BrowserTabActor(this._connection, browser);
> > +  this._actorByBrowser.set(browser, actor);
> > +  this._checkListening();
> > +  return promise.resolve(actor);
> 
> Is it rejected by eslint? I found the original pattern easy to
> read/understand.

Yes, it's this rule: http://eslint.org/docs/rules/no-else-return I don't like it very much either.

> Out of curiosity, does eslint rejects `typeof()`?
> This form is less common, but I actually prefer it as it makes it less
> magical.

typeof is an unary word operator (like new or yield or delete) and space after it is required. The typeof(x) form looks like a function call, which it isn't. http://eslint.org/docs/rules/space-unary-ops

> So eslint isn't able to figure out that if there is a `return` in all
> if/elseif/else patterns, the function actually return an explicit value in
> any case?

Yes, exactly the same case as above.

Comment 20

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Inline sources in iframe stop being displayed after few reloads

Comment 21

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: make devtools/server/actors/webbrowser.js eslint-clean

Addressed review comments:
- added the webbrowser.js as exception to .eslintignore
- use new Error
- improved formatting of comments
- DIDN'T convert methods to ES6 form
- use "Part #" in commit messages, for easier checkin - order matters here

Comment 22

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 3: remove the mustNotifyServiceWorkerRegistrationChanged check as it's dead code

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8745967 [details] [diff] [review]
Part 1: Inline sources in iframe stop being displayed after few reloads

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

Thanks a lot for looking into this non-obvious issue and providing a test!

::: devtools/server/actors/webbrowser.js
@@ +2216,5 @@
>  
>    watch: function(docShell) {
> +    let docShellWindow = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindow);
> +    this._watchedDocShells.add(docShellWindow);

May be put a comment saying that we can't add the docShell in the WeakSet and instead we workaround by storing the window.

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 8745969 [details] [diff] [review]
Part 2: make devtools/server/actors/webbrowser.js eslint-clean

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

Any particular reason to not convert to es6 function definition?
That would be great to cleanup these lines only once.

Comment 25

[Jarda Snajdr [:jsnajdr]]

(In reply to Alexandre Poirot [:ochameau] from comment #24)
> Any particular reason to not convert to es6 function definition?
> That would be great to cleanup these lines only once.

It's just I wanted to do something more exciting today instead of another code cleanup :) But now that the news from bug 1263258 arrived, I'll have to revisit the file once again before landing anyway.

Comment 26

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: Inline sources in iframe stop being displayed after few reloads

Added comment about why docShell can be added to WeakSet.

Comment 27

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: make devtools/server/actors/webbrowser.js eslint-clean

Converted methods to ES6 style, adapted to "function ()" style from bug 1263258.

Comment 28

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 3: remove the mustNotifyServiceWorkerRegistrationChanged check as it's dead code

Changed reviewer to :janx, rebased.

Comment 29

[Alexandre Poirot [:ochameau]]

Comment on attachment 8746483 [details] [diff] [review]
Part 2: make devtools/server/actors/webbrowser.js eslint-clean

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

Thanks!

r+, assuming you have a green try for this.

Comment 30

[Alexandre Poirot [:ochameau]]

Comment on attachment 8746484 [details] [diff] [review]
Part 3: remove the mustNotifyServiceWorkerRegistrationChanged check as it's dead code

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

Looks good, this cleanup now lives in ServiceWorkerRegistrationActorList.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bb1df86bbb35
https://hg.mozilla.org/integration/fx-team/rev/f0222c4acdfa
https://hg.mozilla.org/integration/fx-team/rev/2a1bb1bceccb

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb1df86bbb35
https://hg.mozilla.org/mozilla-central/rev/f0222c4acdfa
https://hg.mozilla.org/mozilla-central/rev/2a1bb1bceccb