Closed Bug 1255066 Opened 5 years ago Closed 5 years ago

[e10s] convert test_bug_461710_perwindowpb.html to mochitest-browser

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: mak, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

Since this test depends on private browsing and multiple windows it's easier to convert it to a mochitest-browser test.
will likely change owner, but for now I'm tracking it
Assignee: nobody → mak77
Attached patch Patch (obsolete) — Splinter Review
Panos asked me to pick this up. The patch converts the test to a mochitest-browser one, and the test passes for me locally with |mach mochitest --e10s|. I'll push to try and post a link here when the results come in.


Marco, I'm not sure who the right reviewer is for this, could you please set the r? flag?
Assignee: mak77 → nhnt11
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
I can review this.
Flags: needinfo?(mak77)
Attachment #8729326 - Flags: review?(mak77)
Comment on attachment 8729326 [details] [diff] [review]
Patch

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

you may need to unbitrot changes to moz.build, chrome.ini and browser.ini (I just landed another test conversion)

::: toolkit/components/places/tests/browser/browser.ini
@@ +21,5 @@
>  [browser_visited_notfound.js]
>  [browser_visituri.js]
>  [browser_visituri_nohistory.js]
>  [browser_visituri_privatebrowsing_perwindowpb.js]
> +[browser_bug461710.js]
\ No newline at end of file

please retain alphabetical order in tests manifests.

::: toolkit/components/places/tests/browser/browser_bug461710.js
@@ +1,1 @@
> +waitForExplicitFinish();

please move this into test(), so it will be more visible when we convert the test to add_task.

@@ +1,3 @@
> +waitForExplicitFinish();
> +
> +Cu.import("resource://gre/modules/NetUtil.jsm");

No more needed, this is now already imported by head.js

@@ +17,5 @@
> + *
> + *    onWaitComplete:  a callback which will be notified when fn() returns
> + *        true.
> + */
> +function waitForTrue(fn, onWaitComplete) {

please, instead of this, use BrowserTestUtils.waitForCondition, so we remove this duped function.

@@ +154,5 @@
> +}
> +
> +function testOnWindow(aIsPrivate, callback) {
> +  var win = OpenBrowserWindow({private: aIsPrivate});
> +  whenDelayedStartupFinished(win, function() { callback(win); });

And this should be replaced with BrowserTestUtils.openNewBrowserWindow that will also wait for delayed-startup.

@@ +189,5 @@
> +  testOnWindow(false, function(aWin) {
> +    var selectedBrowser = aWin.gBrowser.selectedBrowser;
> +
> +     normalWindow = aWin;
> +     normalWindowIframe = selectedBrowser.contentDocument.getElementById("iframe");

This is not OK in e10s, contentDocument is a cpow, we should not use it...

You need ContentTask.spawn to use content, that will require some refactoring

@@ +195,5 @@
> +    testOnWindow(true, function(aPrivateWin) {
> +      selectedBrowser = aPrivateWin.gBrowser.selectedBrowser;
> +
> +      privateWindow = aPrivateWin;
> +      privateWindowIframe = selectedBrowser.contentDocument.getElementById("iframe");

ditto
Attachment #8729326 - Flags: review?(mak77) → review-
Thanks for the review!

(In reply to Marco Bonardo [::mak] from comment #4)
> Comment on attachment 8729326 [details] [diff] [review]
> Patch
> 
> Review of attachment 8729326 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +189,5 @@
> > +  testOnWindow(false, function(aWin) {
> > +    var selectedBrowser = aWin.gBrowser.selectedBrowser;
> > +
> > +     normalWindow = aWin;
> > +     normalWindowIframe = selectedBrowser.contentDocument.getElementById("iframe");
> 
> This is not OK in e10s, contentDocument is a cpow, we should not use it...

D'oh! I've converted most of the test locally to use ContentTask and BrowserTestUtils, but I can't figure out a way to call getVisitedDependentComputedStyle from a ContentTask.

I tried this:

> yield ContentTask.spawn(gTestWindow.gBrowser.selectedBrowser, null, function* () {
>   var doc = content.document;
>   var win = doc.defaultView;
>   function getColor(doc, win, id) {
>     var elem = doc.getElementById(id);
>     var utils = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
>     return utils.getVisitedDependentComputedStyle(elem, "", "color");
>   }
>   is(getColor(doc, win, "link"), kBlue, "Visited link coloring should not work inside of private mode");
> });

This throws at the getVisitedDependentComputedStyle call, with "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)". I can't figure out another way to do this. Do you have any ideas? Am I missing something?

Thanks.
Flags: needinfo?(mak77)
Attached patch Patch v2 (obsolete) — Splinter Review
Figured it out, turns out I was just being silly.

This patch async-ifies stuff with BrowserTestUtils and ContentTasks.
Attachment #8729326 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attachment #8730646 - Flags: review?(mak77)
Comment on attachment 8730646 [details] [diff] [review]
Patch v2

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

if this lands after bug 1254592, you may need to still do some changes, I think at that point chrome.ini will be empty and could be removed (and removed from moz.build)

I'm pretty sure this test can be refactored in half the size, using a simple for/of loop, and avoiding the testNum "dance".  If you wish to look into that, it would be great, but I won't block on it, so do what you feel like.
I was thinking to something like:
let tests = [
  {win: normalWindow, topic: "uri-visit-saved" },
  {win: normalWindow, topic: URI_VISITED_RESOLUTION_TOPIC,
   color: kRed, message: "Visited link coloring should work outside of private mode"},
  ...
]
for (let test of tests) {
  yield ...
  if (test.color) {
    yield ...
  }
}

::: toolkit/components/places/tests/browser/browser_bug461710.js
@@ +41,5 @@
> +
> +  if (testNum == 1)
> +    observer.expectURL(prefix + subtests[0], "uri-visit-saved");
> +  else
> +    observer.expectURL(prefix + subtests[0]);

While here, please brace this if/else

@@ +43,5 @@
> +    observer.expectURL(prefix + subtests[0], "uri-visit-saved");
> +  else
> +    observer.expectURL(prefix + subtests[0]);
> +
> +  let promise = BrowserTestUtils.waitForCondition(() => observer.resolved);

This can be improved a lot using promises. This is a poor implementation of a promise indeed.

The simpler way would be to have expectURL set observer.deferred = PromiseUtils.defer(); and here just yield observer.deferred.promise;
A more sophisticated way could be to completely remove observer and here do something like:

let promise = new Promise(resolve => {
  let uri = NetUtil.newURI(url);
  let topic = testNum == 1 ? "uri-visit-saved" : URI_VISITED_RESOLUTION_TOPIC;
  Services.obs.addObserver(function observe(subject) {
    if (uri.equals(subject.QueryInterface(Ci.nsIURI))) {
      Services.obs.removeObserver(observe, topic);
      resolve();
    }
  }, topic, false);
});

That is far more compact and readable.
In general, don't be shy of refactoring testing code when it's poorly written.

@@ +54,5 @@
> +
> +  yield handleLoad();
> +});
> +
> +var doColorTest = Task.async(function* (aShouldWork) {

aShouldWork is not a great name, I really hope the test works :)
Let's just pass (color, message) as arguments and move their definitions to the caller, that will make the tests more explicit.

@@ +136,5 @@
> +var normalWindow;
> +var privateWindow;
> +
> +add_task(function* () {
> +  waitForExplicitFinish();

you should never mixup add_task and waitForExplicitFinish/finish, it is a good recipe for intermittent failures. add_task by itself already provides a way to asynchronously wait for something, and we should use that.
Attachment #8730646 - Flags: review?(mak77)
Attached patch Patch v3 (obsolete) — Splinter Review
This patch does not refactor the test, but only adds the braces you asked for and gets rid of waitForExplicitFinish.
Attachment #8730646 - Attachment is obsolete: true
Attached patch Patch (refactored test) (obsolete) — Splinter Review
This patch refactors the test as you suggested, but there's a problem. After setting the src of the iframe, it appears that the color of the link changes to match the rules for the :visited state *after* the iframe's contentDocument's load event.

I can't find any way to detect or wait for this change. The other patch uses waitForCondition, which checks the observer's resolved condition every 100ms. This interval seems enough for the color to get updated. The existing code (without either of these patches applied) does the same thing but with an interval of 20ms, and that works too.

I arrived at this conclusion after spending way too much time trying to get to the bottom of why the test was failing (but inconsistently), and the only solution I could think of was manually waiting for a fixed interval of time before doing the color test. Please let me know if you have a better solution.
Marco, I haven't been able to get further than what I said above. If you know a way to detect that color change, that's great. Otherwise, I would prefer leaving the test un-refactored for now, and filing a bug to refactor it and get to the bottom of the issue in that one.

FWIW, I realize that this test can easily start failing intermittently with either of the patches (or in its current state in the tree for that matter) for example if the waiting interval changes or something.
Flags: needinfo?(mak77)
So far, waiting for visited-status-resolution was enough, cause we were notifying it just after updating links, in the same process.
I fear on e10s that doesn't work, cause we notify that topic after sending an async message that WILL update the links.
We notify it here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp#589
the call updating links is history->NotifyVisited(mURI); some rows above.

I'm not sure there's a clean way to wait for all of the messages to be received by the content process, so we'll still have to do with waitForCondition... you could replace the:
is(color, test.color, test.message);
with
// In e10s waiting visited-status-resolution is not enough to ensure links
// have been updated, cause it only tells us messages to update links have
// been dispatched. We must still wait for the actual links update.
info(test.message);
waitForCondition(... and here poll for the color.
In case of failure the test will start timing out, but that's ok.

The test looks MUCH better.
Flags: needinfo?(mak77)
The color is obtained from within a ContentTask, so polling for it needs to be async. This patch modifies BrowserTestUtils.waitForCondition to allow that in a way that does not break any existing consumers of waitForCondition.
Attachment #8731945 - Attachment is obsolete: true
Attachment #8731949 - Attachment is obsolete: true
Attachment #8732405 - Flags: review?(mak77)
Attached patch Part 2 - refactor test (obsolete) — Splinter Review
Brilliant, thanks for the suggestion. This polls for the color.
Attachment #8732406 - Flags: review?(mak77)
Comment on attachment 8732405 [details] [diff] [review]
Part 1 - Allow async generator functions in BrowserTestUtils.waitForCondition

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

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +994,5 @@
>          }
>  
>          let conditionPassed = false;
>          try {
> +          conditionPassed = yield Task.spawn(condition);

why do you need a Task.spawn here? wouldn't just yielding on the function or generator work the same?

It would be different if you'd not be in Task.async, and then you'd need to Task.spawn(condition).then(success, fail);
Comment on attachment 8732406 [details] [diff] [review]
Part 2 - refactor test

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

Looks great

::: toolkit/components/places/tests/browser/browser_bug461710.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: feel free to remove the license header if you're fine sharing this with a PD license. Since it's a rewrite, it may be a good time to relicense.

@@ +62,5 @@
> +    yield promise;
> +
> +    if (test.color) {
> +      // In e10s waiting for visited-status-resolution is not enough to ensure links
> +      // have been updated, because it only tells us that messages to update links 

trailing space
Attachment #8732406 - Flags: review?(mak77) → review+
see comment 14.
Flags: needinfo?(nhnt11)
Yielding on the function directly works fine.
Attachment #8732405 - Attachment is obsolete: true
Attachment #8732405 - Flags: review?(mak77)
Flags: needinfo?(nhnt11)
Attachment #8734113 - Flags: review?(mak77)
Forgot to qref, bah. Sorry.
Attachment #8734113 - Attachment is obsolete: true
Attachment #8734113 - Flags: review?(mak77)
Attachment #8734143 - Flags: review?(mak77)
Removed license and trailing space.
Attachment #8732406 - Attachment is obsolete: true
Attachment #8734152 - Flags: review+
Comment on attachment 8734143 [details] [diff] [review]
Part 1 v2 - Allow async generator functions in BrowserTestUtils.waitForCondition

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

Thank you!
Attachment #8734143 - Flags: review?(mak77) → review+
You need to log in before you can comment on or make changes to this bug.