Closed Bug 1249332 Opened 4 years ago Closed 4 years ago

[e10s] Make browser/modules/test/browser_ContentSearch.js work under e10s

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Make browser/modules/test/browser_ContentSearch.js work under e10s.
Blocks: fxe10s
Blocks: e10s-tests
tracking-e10s: --- → +
Attached patch patch (obsolete) — Splinter Review
Marco, would you mind reviewing?

The only e10s problem was the webprogresslistener used by the test to stop search pages from loading.  This patch moves that into the content script and then has the content script tell the main test about the load, so that the test can continue.
Attachment #8721554 - Flags: review?(mak77)
Attachment #8721554 - Flags: review?(mak77) → review+
Attached patch patch v2Splinter Review
This is strange, the try results were fine (-u mochitest-bc,mochitest-e10s-bc).  The failures on fx-team were actually due to a JS error in waitForLoadAndStopIt, preventing it from stopping the load:

 16:16:05     INFO -  JavaScript error: chrome://mochitests/content/browser/browser/modules/test/contentSearch.js, line 60: TypeError: docShell.addProgressListener is not a function
 16:16:06     INFO -  FATAL ERROR: Non-local network connections are disabled and a connection attempt to www.google.com (74.125.239.114) was made.

And the test passed for me locally in both e10s and non-e10s.  But after refreshing my tree just now, non-e10s fails locally with the error above.

This new patch does a QI dance, like tab-content.js does for the same thing, and it passes locally.  We'll see what try says this time.

Diff between this patch and the previous:

> diff --git a/browser/modules/test/contentSearch.js b/browser/modules/test/contentSearch.js
> --- a/browser/modules/test/contentSearch.js
> +++ b/browser/modules/test/contentSearch.js
> @@ -33,30 +33,33 @@ addMessageListener(TEST_MSG, msg => {
>          url: url,
>        });
>      });
>    }
>  });
> 
>  function waitForLoadAndStopIt(expectedURL, callback) {
>    let Ci = Components.interfaces;
> +  let webProgress =
> +    docShell.QueryInterface(Ci.nsIInterfaceRequestor).
> +    getInterface(Ci.nsIWebProgress);
>    let listener = {
>      onStateChange: function (webProg, req, flags, status) {
>        if (req instanceof Ci.nsIChannel) {
>          let url = req.originalURI.spec;
>          dump("waitForLoadAndStopIt: onStateChange " + url + "\n");
>          let docStart = Ci.nsIWebProgressListener.STATE_IS_DOCUMENT |
>                         Ci.nsIWebProgressListener.STATE_START;
>          if ((flags & docStart) && webProg.isTopLevel && url == expectedURL) {
> -          docShell.removeProgressListener(listener);
> +          webProgress.removeProgressListener(listener);
>            req.cancel(Components.results.NS_ERROR_FAILURE);
>            callback(url);
>          }
>        }
>      },
>      QueryInterface: XPCOMUtils.generateQI([
>        Ci.nsIWebProgressListener,
>        Ci.nsISupportsWeakReference,
>      ]),
>    };
> -  docShell.addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_ALL);
> +  webProgress.addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_ALL);
>    dump("waitForLoadAndStopIt: Waiting for URL to load: " + expectedURL + "\n");
>  }
Attachment #8721554 - Attachment is obsolete: true
Attachment #8722267 - Flags: review?(mak77)
Blocks: 1188692
(In reply to Drew Willcoxon :adw from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c01459669b6

Tons of failures but they're all known and unrelated.
Comment on attachment 8722267 [details] [diff] [review]
patch v2

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

fwiw, I think when the only change is a missing QI, you should just reland without a further review

::: browser/modules/test/contentSearch.js
@@ +39,5 @@
> +function waitForLoadAndStopIt(expectedURL, callback) {
> +  let Ci = Components.interfaces;
> +  let webProgress =
> +    docShell.QueryInterface(Ci.nsIInterfaceRequestor).
> +    getInterface(Ci.nsIWebProgress);

nit: indent as
let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
	                  .getInterface(Ci.nsIWebProgress);
Attachment #8722267 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/f587e6297384
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.