Closed Bug 1353910 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mrbkap)

References

Details

Attachments

(1 file, 11 obsolete files)

Bug 1353910 - Fix intermittents in webrtc tests while removing code duplication.
59 bytes, text/x-review-board-request
florian
: review+
Details 
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)
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)
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)
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: gkrizsanits → mrbkap
I'm going to laugh if this ends up fixing the various intermittents in browser_devices_get_user_media_screen.js too.
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 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 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 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.
Attachment #8855873 - Flags: review?(florian)
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
Attachment #8855466 - Attachment is obsolete: true
Attachment #8855466 - Flags: review?(florian)
Attachment #8855467 - Attachment is obsolete: true
Attachment #8855467 - Flags: review?(florian)
Attachment #8855468 - Attachment is obsolete: true
Attachment #8855468 - Flags: review?(florian)
Attachment #8855469 - Attachment is obsolete: true
Attachment #8855469 - Flags: review?(florian)
Attachment #8855470 - Attachment is obsolete: true
Attachment #8855470 - Flags: review?(florian)
Attachment #8855471 - Attachment is obsolete: true
Attachment #8855471 - Flags: review?(florian)
Attachment #8855472 - Attachment is obsolete: true
Attachment #8855472 - Flags: review?(florian)
Attachment #8855473 - Attachment is obsolete: true
Attachment #8855473 - Flags: review?(florian)
Attachment #8855872 - Attachment is obsolete: true
Attachment #8855872 - Flags: review?(florian)
Attachment #8855873 - Attachment is obsolete: true
Attachment #8855873 - Flags: review?(florian)
Attachment #8855874 - Attachment is obsolete: true
Attachment #8855874 - Flags: review?(florian)
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+
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
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: