1129697 - browser_bookmarksProperties.js and others after it are going to permafail when Gecko 38 merges to Aurora

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Failing tests when Gecko 38 is uplifted to Aurora.

I actually hit this same bustage today when I tried to uplift bug 1115276 and bug 1126633 to Aurora to fix intermittent failures there.

https://treeherder.mozilla.org/logviewer.html#?job_id=4699501&repo=try

10:33:04 INFO - 1243 INFO TEST-START | browser/components/places/tests/browser/browser_bookmarksProperties.js
10:33:04 INFO - 1244 INFO checking window state
10:33:04 INFO - 1245 INFO TEST-PASS | browser/components/places/tests/browser/browser_bookmarksProperties.js | PlacesUtils in context
10:33:04 INFO - 1246 INFO TEST-PASS | browser/components/places/tests/browser/browser_bookmarksProperties.js | PlacesUIUtils in context
10:33:04 INFO - 1247 INFO Start of test: Bug 479348 - Properties on a root should be read-only
10:33:04 INFO - 1248 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmarksProperties.js | uncaught exception - ReferenceError: executeSoon is not defined at chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js:23
10:33:04 INFO - Stack trace:
10:33:04 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1476
10:33:04 INFO - null:null:0
10:33:04 INFO - JavaScript error: chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js, line 23: ReferenceError: executeSoon is not defined
10:33:04 INFO - 1249 INFO Console message: [JavaScript Error: "ReferenceError: executeSoon is not defined" {file: "chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js" line: 23}]

Comment 1

[Marco Bonardo [:mak]]

looks like browser_555547.js leaks into other tests...

hm I wonder if the problem is the fact now the test is using both waitForExplicitFinish AND add_task... it should probably not.

Comment 2

[Marco Bonardo [:mak]]

we should also make waitForExplicitFinish throw if we are inside a task... Sounds like error-prone. Should be detectable by checking if __tasks is defined inside the waitForExplicitFinish function in browser-test.js

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Setting the version to 38.0a2 should suffice to reproduce this locally. That'll make it so that NIGHTLY_BUILD isn't set.
http://hg.mozilla.org/mozilla-central/file/58ce6051edf5/browser/config/version.txt

If that doesn't work, let me know and I can share with you the exact patch I use in the Try runs.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1129697-Fix-browser_555547.js.patch

Could reproduce the failure locally when building Aurora. With the patch it doesn't seem to fail anymore. I don't exactly know why it fails on Aurora and how we could ensure it doesn't with the uplift.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #3)
> Setting the version to 38.0a2 should suffice to reproduce this locally.

Yup, breaks it for me locally. And the patch fixes it.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8559783 [details] [diff] [review]
0001-Bug-1129697-Fix-browser_555547.js.patch

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

I think we want a bug to investigate making browser-test detect waitForExplicitFinish misuse

::: browser/components/places/tests/browser/browser_555547.js
@@ +15,4 @@
>      yield promiseSetToolbarVisibility(toolbar, true);
> +  }
> +
> +  let sidebar = yield promiseLoadedSidebar();

worth to registerCleanupFunction a toggleSidebar() call?

@@ +58,5 @@
> +      sidebar.removeEventListener("load", onLoad, true);
> +      resolve(sidebar);
> +    }, true);
> +
> +    toggleSidebar("viewBookmarksSidebar", true);

I think passing the sidebar name as argument would make the code more readable:

promiseLoadedSidebar("viewBookmarksSidebar");

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/78fe0a9ea65b

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> we should also make waitForExplicitFinish throw if we are inside a task...
> Sounds like error-prone. Should be detectable by checking if __tasks is
> defined inside the waitForExplicitFinish function in browser-test.js

Filed bug 1129936.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78fe0a9ea65b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2931fcc7178

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Untracking for 38 since this was fixed while 38 was in Nightly.