Closed Bug 1069033 Opened 5 years ago Closed 5 years ago

TabTarget doesn't return a window in e10s

Categories

(DevTools :: Framework, defect)

defect
Not set
Points:
3

Tracking

(e10s+)

RESOLVED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
e10s + ---

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 2 obsolete files)

When run in e10s, the browser_target_events test times out, due to it not receiving a 'will-navigate' message.  It turns out this is because the TabWebProgressListener.onStateChange() method fails in the comparison 'if (this.target && this.target.window == progress.DOMWindow) {' - because the .window getter is returning null - because e10s tabs don't have a |contentWindow| property.

Changing this .window getter to be:

  get window() {
    // Be extra careful here, since this may be called by HS_getHudByWindow
    // during shutdown.
    if (this._tab && this._tab.linkedBrowser) {
      let browser = this._tab.linkedBrowser;
      return browser.isRemoteBrowser ? browser.contentWindowAsCPOW : browser.contentWindow;
    }
    return null;
  },

allows this test - and most other tests in browser/devtools/framework pass.

To repro:

% ./mach mochitest-devtools --e10s browser/devtools/framework/test/browser_target_events.js

It times out without this patch but passes with it.
Attachment #8491154 - Flags: review?(past)
Zombie educated me that contentWindowAsCPOW also works for non-remote browsers...
Attachment #8491154 - Attachment is obsolete: true
Attachment #8491154 - Flags: review?(past)
Attachment #8491162 - Flags: review?(past)
Comment on attachment 8491162 [details] [diff] [review]
0001-Bug-XXXXXXX-Make-devtool-s-TabTarget-be-e10s-friendl.patch

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

I'm fine with that, but if I am not mistaken billm said in another bug that he wanted to avoid introducing CPOWs to devtools code. I might be wrong about that, so asking for feedback for my own education, too.
Attachment #8491162 - Flags: review?(past)
Attachment #8491162 - Flags: review+
Attachment #8491162 - Flags: feedback?(wmccloskey)
There are only 2 .window consumers in target itself - one is involved in making the child-process-tab "remote" and the other is simply comparing it to another window (which is the part this bug needs).  Panos, how viable is it to *drop* the window getter and replace it with non-cpowey things?
We have an isTopLevel attribute on nsIWebProgress for this purpose:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgress.idl#140

I think we should be able to change |if (this.target && this.target.window == progress.DOMWindow)| to |if (progress.isTopLevel)|.

It would be great to get rid of the .window getter entirely. It's really a foot-gun in e10s. The other use is already semi-broken in e10s, but it's harder to fix. Mark, it would be great if you could add some sort of warning above it at least.
This patch implements Bill's suggestion: it uses |progress.isTopLevel| in the progress listener, and issues a warning in the .message getter.  This warning is fairly dumb:

+    // XXX - this is a footgun for e10s - there .contentWindow will be null,
+    // and even though .contentWindowAsCPOW *might* work, it will not work
+    // in all contexts.  Consumers of .window need to be refactored to not
+    // rely on this.
+    if (Services.appinfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
+      Cu.reportError("The .window getter on devtools' |target| object isn't e10s friendly!\n"
+                     + new Error().stack + "\n");
+    }

ie, it uses Cu.reportError, but is only issued when e10s is enabled - I'm worried about console spew otherwise.  Another possibility was to keep a boolean and only issue the warning once, but that seems overkill.
Attachment #8491162 - Attachment is obsolete: true
Attachment #8491162 - Flags: feedback?(wmccloskey)
Attachment #8492889 - Flags: review?(past)
Comment on attachment 8492889 [details] [diff] [review]
0001-Bug-1069033-Make-devtool-s-TabTarget-be-e10s-friendl.patch

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

I don't mind replacing the .window getter with something else, but I don't know off the top of my head what that would entail. A quick search for target.window in browser/devtools shows only a few uses in the console and in various test headers. Aren't we likely to be hitting these paths in some of the devtools tests that are already failing in e10s? If so, that warning won't be long-lived.

::: browser/devtools/framework/target.js
@@ +225,5 @@
> +    // in all contexts.  Consumers of .window need to be refactored to not
> +    // rely on this.
> +    if (Services.appinfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +      Cu.reportError("The .window getter on devtools' |target| object isn't e10s friendly!\n"
> +                     + new Error().stack + "\n");

Nit: in case you care about that sort of thing, "Error().stack" is even shorter. Also, I think Error().stack always ends with a newline, so you could omit the extra one.
Attachment #8492889 - Flags: review?(past) → review+
Thanks!

https://hg.mozilla.org/integration/fx-team/rev/8a37303c15ca
Status: NEW → ASSIGNED
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mhammond
Iteration: --- → 35.2
https://hg.mozilla.org/mozilla-central/rev/8a37303c15ca
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.