Closed Bug 1060315 Opened 10 years ago Closed 10 years ago

Intermittent browser_devices_get_user_media.js | video/audio global indicator attribute as expected - Got , expected true

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Iteration:
36.2
Tracking Flags:
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: cbook, Assigned: florian)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Patch with yield*
3.90 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
Ubuntu VM 12.04 mozilla-inbound pgo test mochitest-browser-chrome-1 on 2014-08-29 02:13:51 PDT for push 5f66dd3d63f2

slave: tst-linux32-spot-574

https://tbpl.mozilla.org/php/getParsedLog.php?id=47017529&tree=Mozilla-Inbound



633 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_devices_get_user_media.js | video global indicator attribute as expected - Got , expected true
Blocks: 1041658
I wonder if the fix for bug 973001 increased the frequency of this intermittent failure.
Summary: Intermittent browser_devices_get_user_media.js | video global indicator attribute as expected - Got , expected true → Intermittent browser_devices_get_user_media.js | video/audio global indicator attribute as expected - Got , expected true
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> I wonder if the fix for bug 973001 increased the frequency of this
> intermittent failure.

Looking at war on orange, I'm seeing a spike around Oct 12/13:

http://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1060315&startday=2014-08-21&endday=2014-10-21&tree=trunk

Do you have time to look at this, Florian, or know someone who does? It is now one of the top oranges...
Flags: needinfo?(florian)
I think the frequency of this failure has actually increased twice. When the bug was initially filed, this was a very rare failure (about once a week). Around September 22, it became more common, failing a few times per day; my guess is that increase was due to the webrtc UI becoming more asynchronous with the fix for e10s.

Then, like you observed, there's another increase in October, making this fail very frequently. I have no idea of the cause of that new increase.

For the reason for the failure, my guess is that the webrtcIndicator.xul window hasn't finished loading when we check DOM attributes on it.

The problem would be near http://hg.mozilla.org/mozilla-central/annotate/29fbfc1b31aa/browser/base/content/test/general/head.js#l669

Possible fix ideas:
- hide the problem with waitForCondition
- check that the document has finished loading (using document.readyState) before the DOM attribute checks. If it hasn't, listen to the readystatechange event.

Is the test now failing frequently enough that pushing a possible fix to try and retriggering bc1 several times can confirm the fix? Not being able to reproduce the failure is the reason why I haven't tried any of these possible fixes a month ago.
Flags: needinfo?(florian)
Disabled for frequent failures.
https://hg.mozilla.org/integration/fx-team/rev/ae72be50c01d
Whiteboard: [test disabled on Linux][leave open]
Attached patch Possible patch (obsolete) — — Splinter Review 
Here is a patch implementing my suggestion from comment 286.

I haven't been able to reproduce the failure locally on my Linux machine. So assuming the attached patch looks reasonable, our options would either be to just check it in and see if the failures disappear, or push to try and retrigger bc1 hundreds of times...
Attachment #8513533 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8513533 [details] [diff] [review]
Possible patch

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

::: browser/base/content/test/general/browser_devices_get_user_media.js
@@ +191,4 @@
>    if (!aAlreadyClosed)
>      expectObserverCalled("recording-window-ended");
>  
> +  yield assertWebRTCIndicatorStatus(null);

I thought yielding for yields in the function call itself needed to be yield* ?
(In reply to :Gijs Kruitbosch from comment #427)

> ::: browser/base/content/test/general/browser_devices_get_user_media.js
> @@ +191,4 @@
> >    if (!aAlreadyClosed)
> >      expectObserverCalled("recording-window-ended");
> >  
> > +  yield assertWebRTCIndicatorStatus(null);
> 
> I thought yielding for yields in the function call itself needed to be
> yield* ?

I'd never seen 'yield*' before. After reading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function* it seems we indeed need yield* here... but then I don't understand why the test works without doing that on closeStream and checkSharingUI.
Comment on attachment 8513533 [details] [diff] [review]
Possible patch

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

As noted on IRC, r+ with yield*, let's see if this helps. Ensure browser.ini only disables this on linux debug, and the linux+windows opt failures should make it pretty obvious whether it works.
Attachment #8513533 - Flags: review?(gijskruitbosch+bugs) → review+
If I use yield*, the test fails with:
16 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_devices_get_user_media.js | Exception thrown - at chrome://mochitests/content/browser/browser/base/content/test/general/browser_devices_get_user_media.js:194 - SyntaxError: expected expression, got '*'


I think the reason why the test worked without using yield* is explained in: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm#Introduction

"If you specify a generator function or the iterator returned by a generator function, then Task.spawn() is implicitly called, and the yield operator works on the returned promise. This reduces the syntax overhead of calling Task.spawn() explicitly when you want to recurse into other task functions."
(In reply to Florian Quèze [:florian] [:flo] from comment #438)
> If I use yield*, the test fails with:
> 16 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_devices_get_user_media.js | Exception thrown - at
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_devices_get_user_media.js:194 - SyntaxError: expected expression,
> got '*'

Wat. That doesn't make sense to me (as in, the docs say this should work, right?), but then, rs=me to land as-is. Oh, although I guess maybe this only works if you've explicitly declared this a generator function (function*) ?
(In reply to :Gijs Kruitbosch from comment #439)
> Oh, although I guess maybe this only
> works if you've explicitly declared this a generator function (function*) ?

Indeed, if I change these to function* the test works locally.

Let me know if you prefer this version of the patch :).
(In reply to Florian Quèze [:florian] [:flo] from comment #440)
> Created attachment 8514210 [details] [diff] [review]
> Patch with yield*
> 
> (In reply to :Gijs Kruitbosch from comment #439)
> > Oh, although I guess maybe this only
> > works if you've explicitly declared this a generator function (function*) ?
> 
> Indeed, if I change these to function* the test works locally.
> 
> Let me know if you prefer this version of the patch :).

I think I prefer the starred version. More stars, more awesome, right?

(more seriously: it really is already a generator function, so we should just use the right syntax, so we don't break when spidermonkey decides to make more things actual syntax errors instead of accepting them anyway (presumably because we used to yield before we had function*?))
https://hg.mozilla.org/integration/fx-team/rev/7a201ad45ce2
Whiteboard: [test disabled on Linux][leave open] → [leave open]
(In reply to TBPL Robot from comment #444)
> submit_timestamp: 2014-10-30T08:18:28
> log:
> https://treeherder.mozilla.org/ui/logviewer.html#?repo=fx-team&job_id=1044288
> repository: fx-team
> who: tomcat[at]mozilla[dot]com
> machine: t-xp32-ix-063
> buildname: Windows XP 32-bit fx-team debug test mochitest-browser-chrome-1
> revision: 1b80d1642142

This is from the last fx-team changeset before my patch.
Comment on attachment 8514210 [details] [diff] [review]
Patch with yield*

It seems to me we should get this uplifted. Ryan, can we just do that or do we need formal approval?
Flags: needinfo?(ryanvm)
Attachment #8514210 - Flags: review+
Attachment #8513533 - Attachment is obsolete: true
Test-only changes don't need approval
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/17b84511a934
Assignee: nobody → florian
Status: NEW → RESOLVED
Points: --- → 3
Closed: 10 years ago
status-firefox35: --- → fixed
status-firefox36: --- → fixed
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → Firefox 36
Flags: qe-verify-
Iteration: --- → 36.2
Flags: firefox-backlog+
The last 3 failures that have been stared here are different from the rest of this bug.

All the other failures here have a message saying "Got , expected true" (note the empty string), indicating we are running the test before something gets initialized. This bug is FIXED.

The 3 newer failures have messages saying "Got true, expected false". That's a different bug. If I had to make a guess, I would say it's a regression from bug 1095733.
Flags: needinfo?(ryanvm)
Flags: needinfo?(philringnalda)
Spun off to bug 1099095.
Flags: needinfo?(ryanvm)
Flags: needinfo?(philringnalda)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: