Permafailing on Aurora since e10s-multi Windows browser_devices_get_user_media_anim.js | the animated sharing icon of the tab is hidden - Got -moz-box, expected none

RESOLVED FIXED in Firefox 54

Status

()

Core
WebRTC
P2
normal
Rank:
25
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Treeherder Bug Filer, Assigned: mrbkap)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 11 obsolete attachments)

59 bytes, text/x-review-board-request
florian
: review+
Details | Review
(Reporter)

Description

a year ago
treeherder
Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=88736208&repo=mozilla-aurora

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-aurora-win32-pgo/1491353187/mozilla-aurora_win7_vm_test_pgo-mochitest-e10s-browser-chrome-4-bm129-tests1-windows-build91.txt.gz

Interestingly, I did a 55-as-Aurora Try simulation and the failures don't happen. But it's currently permafailing on Windows.
Assignee: nobody → gkrizsanits
status-firefox54: --- → affected
Keywords: intermittent-failure
Blake sent me a pastebin of a patch to run through Try and it looks great!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37886d587866e548fab9db8ce03c623ecf20e470
Are there other tests in this directory that need similar modifications?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 3

a year ago
Yes. I would add a function to that directory's head.js that runs an array of tests (and basically looks like the function `test` in this file) that abstracts out the various things the tests need and call that from all of the tests in this directory. I can write the patch tomorrow if need be.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 4

a year ago
I wonder what the difference is between 55 and 54 to cause this to happen. Are we not preloading processes (and therefore making the "create a new tab and switch to it" slower)? The difference between my patch and the current code is that `BTU.withNewTab` waits for switchTab to finish before continuing.
(In reply to Blake Kaplan (:mrbkap) from comment #4)
> I wonder what the difference is between 55 and 54 to cause this to happen.
> Are we not preloading processes (and therefore making the "create a new tab
> and switch to it" slower)? The difference between my patch and the current
> code is that `BTU.withNewTab` waits for switchTab to finish before
> continuing.

We don't preallocate processes on nightly either yet... Either the tab switching became faster or the content process startup in 55 (or both). Since disabling this test made the next one fail and your patch combined with keeping the processes alive fixed all the problems I would bet it's the content process startup. I think Andrew is working on that. Andrew, are there patches that got landed on nightly but did not make it to aurora 54 yet?
Flags: needinfo?(continuation)

Updated

a year ago
Rank: 25
Priority: -- → P2
I haven't been looking at startup time in particular, I've only been reducing the loading of .jsm files. I don't know if that helps startup speed enough to be noticeable. My patches are blocking bug 1350472. I haven't uplifted any to 54.
Flags: needinfo?(continuation)
Maybe Mike has some ideas of what might have improved with content process startup time?
Flags: needinfo?(mconley)
(Assignee)

Updated

a year ago
Assignee: gkrizsanits → mrbkap
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'm going to laugh if this ends up fixing the various intermittents in browser_devices_get_user_media_screen.js too.

Comment 18

a year ago
mozreview-review
Comment on attachment 8855465 [details]
Bug 1353910 - Fix intermittents in webrtc tests while removing code duplication.

https://reviewboard.mozilla.org/r/127320/#review130374

::: browser/base/content/test/webrtc/head.js:524
(Diff revision 1)
>             .location
>             .reload();
>    });
>  }
> +
> +async function runTests(tests, page) {

Could "get_user_media.html" be set as the default value of this 'page' parameter?

::: browser/base/content/test/webrtc/head.js:529
(Diff revision 1)
> +async function runTests(tests, page) {
> +  let rootDir = getRootDirectory(gTestPath);
> +  rootDir = rootDir.replace("chrome://mochitests/content/",
> +                            "https://example.com/");
> +
> +  await BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank" },

Is there a good reason why we need to load about:blank before loading the actual test page?

Comment 19

a year ago
mozreview-review
Comment on attachment 8855466 [details]
Bug 1353910 - Wait for the tab switch (and deduplicate code) in browser_devices_get_user_media_in_frame.js.

https://reviewboard.mozilla.org/r/127322/#review130376

::: browser/base/content/test/webrtc/browser_devices_get_user_media_anim.js:76
(Diff revision 1)
>  }
>  
>  ];
>  
>  add_task(async function test() {
> -  await runTests(gTests, "get_user_media.html");
> +  let rootDir = getRootDirectory(gTestPath);

Why are you reverting to having this rootDir computation in each individual test?

::: browser/base/content/test/webrtc/head.js:542
(Diff revision 1)
>        yield SpecialPowers.pushPrefEnv({"set": [[PREF_PERMISSION_FAKE, true]]});
>  
>        for (let testCase of tests) {
>          info(testCase.desc);
>          yield testCase.run();
> +        if (testCleanup) {

Could we run 'yield expectNoObserverCalled();' here for all tests? If not, could the parameter be only an optional boolean to disable this check when needed?
But I now see that the browser_webrtc_hooks.js test needs to customize it... So maybe expectNoObserverCalled can be the default value of that testCleanup parameter?

Comment 20

a year ago
mozreview-review
Comment on attachment 8855469 [details]
Bug 1353910 - Fix browser_devices_get_user_media_tear_off_tab.js.

https://reviewboard.mozilla.org/r/127328/#review130378

::: browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js:72
(Diff revision 1)
> -  }, {capture: true, once: true});
>    let rootDir = getRootDirectory(gTestPath);
>    rootDir = rootDir.replace("chrome://mochitests/content/",
>                              "https://example.com/");
> -  content.location = rootDir + "get_user_media.html";
> -}
> +
> +  // NB: This causes withNewTab to warn that we already closed the tab. That

What is 'this' in this comment?
Is the warning a bug of withNewTab? Or are we doing something here that would be preferable to avoid?
Thanks for removing all this code duplication, I've meant to do it for a while!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

a year ago
mozreview-review
Comment on attachment 8855469 [details]
Bug 1353910 - Fix browser_devices_get_user_media_tear_off_tab.js.

https://reviewboard.mozilla.org/r/127328/#review130454

::: browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js:72
(Diff revision 1)
> -  }, {capture: true, once: true});
>    let rootDir = getRootDirectory(gTestPath);
>    rootDir = rootDir.replace("chrome://mochitests/content/",
>                              "https://example.com/");
> -  content.location = rootDir + "get_user_media.html";
> -}
> +
> +  // NB: This causes withNewTab to warn that we already closed the tab. That

This test takes the current tab, tears it off into a new window, and the closes that window. `withNewTab` expects that the tab it opens remains open throughout the call to `taskFn`. I've changed `runTests` to do its own verison of `withNewTab` but agnostic as to the tab it ends up closing.
(Assignee)

Updated

a year ago
Attachment #8855873 - Flags: review?(florian)

Comment 26

a year ago
mozreview-review
Comment on attachment 8855873 [details]
Bug 1353910 - Stop using withNewTab to avoid warnings in a couple of tests.

https://reviewboard.mozilla.org/r/127746/#review130494

::: browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js:66
(Diff revision 1)
>  add_task(async function test() {
>    await SpecialPowers.pushPrefEnv({"set": [["dom.ipc.processCount", 1]]});
>  
>    // An empty tab where we can load the content script without leaving it
>    // behind at the end of the test.
>    let tab = gBrowser.addTab();

nit: remove the now unused 'tab' variable.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_tear_off_tab.js:60
(Diff revision 1)
>  add_task(async function test() {
>    await SpecialPowers.pushPrefEnv({"set": [["dom.ipc.processCount", 1]]});
>  
>    // An empty tab where we can load the content script without leaving it
>    // behind at the end of the test.
>    let tab = gBrowser.addTab();

same here
(In reply to Florian Quèze [:florian] [:flo] from comment #26)

> nit: remove the now unused 'tab' variable.

Ah, I see you did that in the next patch.
The whole set of changes looks good to me (ie. r=me), but I'm not convinced we really need the 12 separate changesets; especially when some undo what previous changesets did. And I also don't know how to r+ all of this in mozreview without doing A LOT of clicking around.

Also, if you want to get a feel of how this impacts the intermittents, I would suggest using try with a syntax like:
 try: -b do -p linux64,linux64-asan,linux,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none --rebuild 10 --try-test-paths browser-chrome:browser/base/content/test/webrtc
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8855466 - Attachment is obsolete: true
Attachment #8855466 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855467 - Attachment is obsolete: true
Attachment #8855467 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855468 - Attachment is obsolete: true
Attachment #8855468 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855469 - Attachment is obsolete: true
Attachment #8855469 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855470 - Attachment is obsolete: true
Attachment #8855470 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855471 - Attachment is obsolete: true
Attachment #8855471 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855472 - Attachment is obsolete: true
Attachment #8855472 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855473 - Attachment is obsolete: true
Attachment #8855473 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855872 - Attachment is obsolete: true
Attachment #8855872 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855873 - Attachment is obsolete: true
Attachment #8855873 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8855874 - Attachment is obsolete: true
Attachment #8855874 - Flags: review?(florian)

Comment 30

a year ago
mozreview-review
Comment on attachment 8855465 [details]
Bug 1353910 - Fix intermittents in webrtc tests while removing code duplication.

https://reviewboard.mozilla.org/r/127320/#review130518
Attachment #8855465 - Flags: review?(florian) → review+

Comment 31

a year ago
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8213314b5246
Fix intermittents in webrtc tests while removing code duplication. r=florian
https://hg.mozilla.org/mozilla-central/rev/8213314b5246
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
23 failures in 867 pushes (0.027 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-aurora: 23

Platform breakdown:
* windows8-64: 20
* windows7-32-vm: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1353910&startday=2017-04-03&endday=2017-04-09&tree=all

Comment 34

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/67a28d0a489b
status-firefox54: affected → fixed
Flags: in-testsuite+
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.