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

()

P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

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

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment, 11 obsolete attachments)

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

Description

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8855873 - Flags: review?(florian)

Comment 26

2 years 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

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

2 years ago
Attachment #8855874 - Attachment is obsolete: true
Attachment #8855874 - Flags: review?(florian)

Comment 30

2 years 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

2 years 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: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment hidden (Intermittent Failures Robot)

Comment 34

2 years 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.