1099156 - Fix browser_bug435325.js to work on e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

This test tries to access the net error page in content, to click the 'go online' button, and then to try to check if that actually works. It times out when run under e10s.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

I removed hasBrowserHandlers from tab-content.js since it's unnecessary and misleading. Unlike the browser.js code setting that attribute, this code deals with about:home.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8725636 [details] [diff] [review]
patch

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

Hey dao, thanks for the patch!

I gave a suggestion on how to refactor the test using BrowserTestUtils to make it less callback-y using add_task, but that's optional. What I'm most interested in fixing is just flat-out removing the hasBrowserHandlers stuff entirely (not just for e10s), since it appears to me to be without use.

Let me know if you have questions.

::: browser/base/content/tab-content.js
@@ -149,5 @@
>    },
>  
>    onPageLoad: function() {
> -    let doc = content.document;
> -    if (doc.documentElement.hasAttribute("hasBrowserHandlers")) {

Is hasBrowserHandlers even useful for the non-e10s case? It looks like it's mainly used to make sure we only add a click event handler on a browser at an about: page once...

But that click event handler (BrowserOnClick) doesn't seem to do much. In fact, it seems like it's only used for about:newtab in the e10s case here, and that was added to make sure that we properly flip to the remote case when clicking a tile.

That was added back in bug 907342, and since then, much work has been done to make the remoteness flipping logic more "robust". I believe we can remove the event handler from BrowserOnClick entirely, to no ill effect (local testing seems to confirm this).

So I think this can be simplified by just flat-out removing hasBrowserHandlers wholesale, from here and from browser.js, along with the event handler on BrowserOnClick.

::: browser/base/content/test/general/browser_bug435325.js
@@ +27,5 @@
> +
> +      /* If this is a remote browser, the hasBrowserHandlers attribute
> +         won't be set and the content script should respond to click events
> +         immediately. */
> +      if (${gBrowser.selectedBrowser.isRemoteBrowser}) {

Pretty neat meta programming you've got going on here. ;)

With the removal of hasBrowserHandlers, I don't believe this will be necessary any longer.

In fact, this can all probably be simplified down to:

add_task(function*() {
  // ... put us in offline, disable caches, etc, and register a cleanup function
  // to revert all that stuff.

  gBrowser.selectedTab = gBrowser.addTab("http://example.com/");

  let browser = gBrowser.selectedBrowser;

  yield ContentTask.spawn(browser, null, function*() {
    yield ContentTaskUtils.waitForEvent(content, "DOMContentLoaded");

    // Do the documentURI check here
  });

  // Re-enable the proxy...


  let onlinePromise = TestUtils.topicObserved("network:offline-status-changed");

  yield BrowserTestUtils.synthesizeMouseAtCenter("#errorTryAgain", {}, browser);
  
  yield onlinePromise;

  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
});

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

> So I think this can be simplified by just flat-out removing
> hasBrowserHandlers wholesale, from here and from browser.js, along with the
> event handler on BrowserOnClick.

Please file a new bug on this. It feels both detached and risky enough that I don't want it to be part of this patch.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8726127 [details] [diff] [review]
patch v2

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

Alright, r=me since this gets this working. I've filed bug 1253272 to remove hasBrowserHandlers and bug 1253274 to update the test to use add_task.

Comment 5

[Pulsebot]

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

Comment 6

[Pulsebot]

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

Comment 7

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b5e6ba01157

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e811a988416
https://hg.mozilla.org/mozilla-central/rev/50f6df5b5581