Closed Bug 976544 Opened 11 years ago Closed 7 years ago

Intermittent browser_get_user_media.js,browser_devices_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

Product:
Core
Component:
WebRTC: Audio/Video
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox29 --- unaffected
firefox30 --- disabled
firefox31 --- disabled
firefox-esr24 --- unaffected

People

(Reporter: cbook, Assigned: florian)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: intermittent-failure, Whiteboard: [test disabled on Linux debug])

Attachments

(5 files, 3 obsolete files)

Patch
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
Second patch, v2
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
popupshown approach, v2
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
Rename, and check no previous popup notification
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
Disable on Linux debug
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
Rev5 MacOSX Mountain Lion 10.8 fx-team opt test mochitest-browser-chrome on 2014-02-25 05:11:54 PST for push a87f7974e8fe slave: talos-mtnlion-r5-019 https://tbpl.mozilla.org/php/getParsedLog.php?id=35213320&tree=Fx-Team TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null
I tried to reproduce this intermittent (that I've seen once before on the try server on Linux, as mentioned in bug 804611 comment 52). I haven't been able to reproduce locally (yet). The attached patch is the changes I had to do to browser_get_user_media.js so that it works fine with --run-until-failure --repeat 999999
A guess here is that there could be an XBL binding that isn't reliably attached at the time this line of the test is executed. If this is the correct explanations, workarounds could be either to force a synchronous binding attachment by forcing layout, or to wait for panel.firstChild to be non-null. I would really like to have a way to reproduce locally to test this hypothesis.
My attempts to reproduce this on my local machine (OS X 10.8 with an opt build, so similar to some of the failures here) have failed. If I run the test with --run-until-failure --repeat 999999, after a bit more than 16 minutes, the tests consistently fail with a different error indicating the browser is out of memory. I looked at the about:memory dumps (thanks to --dump-about-memory-after-test --dump-output-directory=<path>) and it turns out that explicit/resident have low values, but vsize is large (> 4GB). I discussed this on #memshrink yesterday evening, but the discussion didn't result in a way to further debug this OOM issue. Given this, I think I won't be able to reproduce the issue on my machine.
Attached patch Patch (deleted) — — Splinter Review
I can't say for sure that this will fix the problem (and I don't understand why this would be needed), but it shouldn't hurt, and the exact same change fixed bug 886995 before.
Attachment #8381399 - Attachment is obsolete: true
Attachment #8382352 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8382352 [details] [diff] [review] Patch Review of attachment 8382352 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the panel popup waiting changes, but I don't understand the value = 0 bits. Can you elucidate? (and, perhaps you should ask for review from the person who originally reviewed these tests? ::: browser/base/content/test/general/browser_get_user_media.js @@ +528,5 @@ > yield checkPerm(true, true, false, true, false, true); > + > + // reset the menuitems to have no impact on the following tests. > + elt("webRTC-selectMicrophone-menulist").value = 0; > + elt("webRTC-selectCamera-menulist").value = 0; Why are these necessary?
Attachment #8382352 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #31) > Comment on attachment 8382352 [details] [diff] [review] > I don't understand the value = 0 bits. > Can you elucidate? (and, perhaps you should ask for review from the > person who originally reviewed these tests? > > ::: browser/base/content/test/general/browser_get_user_media.js > @@ +528,5 @@ > > yield checkPerm(true, true, false, true, false, true); > > + > > + // reset the menuitems to have no impact on the following tests. > > + elt("webRTC-selectMicrophone-menulist").value = 0; > > + elt("webRTC-selectCamera-menulist").value = 0; > > Why are these necessary? 0 is the initial value. These lines are to undo the effect of http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_get_user_media.js#441 and http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_get_user_media.js#447 There's already something really similar at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_get_user_media.js#351 This change isn't to fix the orange, but to let the test work fine with --run-until-failure, as I said in comment 1.
Please let me know if comment 32 is enough of an explanation to checkin the patch with r=you.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Florian Quèze [:florian] [:flo] from comment #33) > Please let me know if comment 32 is enough of an explanation to checkin the > patch with r=you. Yes. Please also ensure that for all/any other changes the test makes, it returns the browser back to normal as much as possible.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #34) Thanks! > Please also ensure that for all/any other changes the test makes, it returns > the browser back to normal as much as possible. Sure, that's what I tried to do, not resetting these 2 values was just a mistake that I'm happy to fix.
Not sure if the bug will actually be fixed, but I guess we will know within a day or two: https://hg.mozilla.org/integration/fx-team/rev/225a7ed5aa99
Assignee: nobody → florian
https://hg.mozilla.org/mozilla-central/rev/225a7ed5aa99
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(In reply to TBPL Robot from comment #55) > philor > https://tbpl.mozilla.org/php/getParsedLog.php?id=35334326&tree=Fx-Team > Rev3 Fedora 12x64 fx-team debug test mochitest-browser-chrome on 2014-02-26 > 20:54:50 > revision: 85fbd0ccd097 > slave: talos-r3-fed64-069 > > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js | timeout waiting for popup notification > webRTC-shareDevices > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js | notification panel populated > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js | Unexpected Exception: TypeError: > PopupNotifications.panel.firstChild is null This failure is with the patch applied. So I'm afraid the patch hasn't fixed the issue, and has at best made it less frequent (although I don't have solid data to prove it). My next idea is to force the panel to reopen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Another attempt (obsolete) (deleted) — — Splinter Review
I'm still unable to reproduce the failure, so... here is another attempt. Like the previous one it shouldn't hurt, but I can't guarantee success either. :-(
Attachment #8383793 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8383793 [details] [diff] [review] Another attempt Review of attachment 8383793 [details] [diff] [review]: ----------------------------------------------------------------- How will this actually still be testing anything? Surely this means that if the code doesn't show the notification to begin with, the test code will then force it to be displayed after the first check times out? That sounds like it won't catch genuine failures anymore... And actually, looking some more, it seems from e.g. comment 90 that the initial waitForCondition times out, and you're not changing that condition so that presumably means that that failure would still happen, which means the test will still be orange. I think instead, you should add a popuphidden event listener (and remove it afterwards, of course), and reshow from there. Note that that means you have to set up the promise/yield code *before* you trigger the notification (and that code should assert that the panel isn't already opened). Finally, I'll just note that it seems in many of the other cases where this fails, some previous test's still-open windows cause a focus change. On OS X that might also be a fullscreen transition (which takes time, but the .fullScreen property changes immediately - separate bug). Have you looked at the tests that run before this one to see if something like that could be causing these failures? Fixing that would lead to less hacky patches.
Attachment #8383793 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #95) > How will this actually still be testing anything? What I really want to test with this test file is that the notification is displayed (which is checked by the fact that the notification icon is found), that its content is correct, and that user actions on it have the expected effect. I'm not interested in testing that PopupNotifications works, because I'm hoping that test/general/browser_popupNotification.js already does a far better job at it than my test here could. > And actually, looking some more, it seems from e.g. comment 90 that the > initial waitForCondition times out, and you're not changing that condition > so that presumably means that that failure would still happen, which means > the test will still be orange. Good catch, I should have removed the (!aShown || PopupNotifications.panel.firstChild) part of the previous waitForCondition. > I think instead, you should add a popuphidden event listener (and remove it > afterwards, of course), and reshow from there. This would work, but is more complicated and I don't see how it's better (I guess that depends if you agree with my answer to your first question). > Finally, I'll just note that it seems in many of the other cases where this > fails, some previous test's still-open windows cause a focus change. On OS X > that might also be a fullscreen transition (which takes time, but the > .fullScreen property changes immediately - separate bug). Have you looked at > the tests that run before this one to see if something like that could be > causing these failures? Fixing that would lead to less hacky patches. This would be a good explanation; 2 tests before mine there's a test about fullscreen: 267 [browser_fullscreen-window-open.js] 268 [browser_gestureSupport.js] 269 [browser_get_user_media.js]
(In reply to Florian Quèze [:florian] [:flo] from comment #99) > (In reply to :Gijs Kruitbosch from comment #95) > > > How will this actually still be testing anything? > > What I really want to test with this test file is that the notification is > displayed (which is checked by the fact that the notification icon is > found), that its content is correct, and that user actions on it have the > expected effect. > > I'm not interested in testing that PopupNotifications works, because I'm > hoping that test/general/browser_popupNotification.js already does a far > better job at it than my test here could. > > > And actually, looking some more, it seems from e.g. comment 90 that the > > initial waitForCondition times out, and you're not changing that condition > > so that presumably means that that failure would still happen, which means > > the test will still be orange. > > Good catch, I should have removed the (!aShown || > PopupNotifications.panel.firstChild) part of the previous waitForCondition. > > > I think instead, you should add a popuphidden event listener (and remove it > > afterwards, of course), and reshow from there. > > This would work, but is more complicated and I don't see how it's better (I > guess that depends if you agree with my answer to your first question). Makes sense to me. Have you checked what that other test does and if you've missed anything? > > Finally, I'll just note that it seems in many of the other cases where this > > fails, some previous test's still-open windows cause a focus change. On OS X > > that might also be a fullscreen transition (which takes time, but the > > .fullScreen property changes immediately - separate bug). Have you looked at > > the tests that run before this one to see if something like that could be > > causing these failures? Fixing that would lead to less hacky patches. > > This would be a good explanation; 2 tests before mine there's a test about > fullscreen: > 267 [browser_fullscreen-window-open.js] > 268 [browser_gestureSupport.js] > 269 [browser_get_user_media.js] This sounds like it might be worth pursuing as well.
This is currently #1 on Orange Factor, so please can we fix or disable this test soon?
Flags: needinfo?(florian)
I'll provide an updated version of attachment 8383793 [details] [diff] [review] today.
Flags: needinfo?(florian)
Attached patch Second patch, v2 (deleted) — — Splinter Review
I've just removed the broken condition that would have caused the test to still timeout in the previous attachment; while not completely satisfying, I think this is the best way to get rid of the orange soon. (In reply to :Gijs Kruitbosch from comment #138) > Have you checked what that other test does and if you've > missed anything? It looks like this other test is using a popupshown event listener to execute its test code. I suspect this means the test code is running synchronously as soon as the popup is shown; even if the popup gets discarded immediately because the window loses focus. I'm not sure though, that's just a guess after looking at the code.
Attachment #8383793 - Attachment is obsolete: true
Attachment #8385629 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8385629 [details] [diff] [review] Second patch, v2 Review of attachment 8385629 [details] [diff] [review]: ----------------------------------------------------------------- I guess in an effort to get this test to not be so orange I'm OK with this, but it sounds like the popupshown handler would be a less hackish way to fix this issue.
Attachment #8385629 - Flags: review?(gijskruitbosch+bugs) → review+
This seems like a rather bad hackaround. Have we really exhausted possibilities for why the popup is disappearing? Can we add additional logging to try to figure out what's going on?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #161) > Have we really exhausted possibilities for why the popup is disappearing? Not really. The idea about fullscreen transitions being the cause seems plausible, especially as browser_fullscreen-window-open.js is executed near this test. However, I think we have exhausted our sheriff's patience. > Can we add additional logging to try to figure out what's going on? There's an additional todo() call when the test would have failed before. Do we have an automated way to detect test runs that have additional EXPECTED-FAIL lines in their logs?
(In reply to TBPL Robot from comment #172) > philor > https://tbpl.mozilla.org/php/getParsedLog.php?id=35643753&tree=Fx-Team > Rev3 Fedora 12x64 fx-team debug test mochitest-browser-chrome on 2014-03-04 > 20:03:16 > revision: ee131fc23afd > slave: talos-r3-fed64-060 > > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js | timeout waiting for notification to be reshown This message has been added by the latest patch, so the orange is still not fixed :-(. I guess I'll look into what's going one with the fullscreen test.
https://hg.mozilla.org/mozilla-central/rev/6622c62be1d5
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 30 → ---
The focus issue being caused by the fullscreen test is plausible. The test that runs between the fullscreen test and mine (browser_gestureSupport.js) is short (less than 200ms). At the end of the fullscreen test, I see this line in the test log: 01:37:55 INFO - TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_fullscreen-window-open.js | must wait for focus So it seems like indeed the window isn't focused immediately at the end of the fullscreen test... but testing/mochitest/browser-test.js seems to correctly take care of focusing it again; so I don't see anything I could fix there. I'm still unable to reproduce locally. I tried using the try server to reproduce: I commented out all the tests that are executed after mine, so that 'bc' runs finish quickly ( < 5 minutes). Unfortunately, despite retriggering more than 100 times, I didn't get a single failure of the test: https://tbpl.mozilla.org/?tree=Try&rev=385d92b2805f I guess the next step here is to try the popupshown event listener solution, but I'm afraid that will be yet another fix attempt without proof that it actually fixes the issue, until we see if the orange still happens after a few days.
Attached patch bug976544-4.patch (obsolete) (deleted) — — Splinter Review
This replaces the hacks of the previous 2 patches with the popupshown approach that other tests seem to be using. The tests pass locally... but they also passed before. I pushed this to try at https://tbpl.mozilla.org/?tree=Try&rev=5f13bfedba04
Attachment #8386777 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8386777 [details] [diff] [review] bug976544-4.patch Review of attachment 8386777 [details] [diff] [review]: ----------------------------------------------------------------- Got some questions: ::: browser/base/content/test/general/browser_get_user_media.js @@ +94,5 @@ > + > + return deferred.promise; > +} > + > +function promisePopupNotification(aName) { This function is dead now, isn't it? Please remove it if that is the case - if not, why isn't it? :-) @@ +223,4 @@ > info("requesting devices"); > content.wrappedJSObject.requestDevice(true, true); > }); > + expectNotification("getUserMedia:request"); These seem new and decrement a global counter, can you explain why you added them and/or how it's related to the random orange?
Attachment #8386777 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #184) > Comment on attachment 8386777 [details] [diff] [review] > > +function promisePopupNotification(aName) { > > This function is dead now, isn't it? It is used in checkSharingUI, which is called for each of the tests. It's handling the case where before I had aShown=false. The webRTC-shareDevices popup notification requests something from the user, so the popup is shown by default. The webRTC-sharingDevices popup notification is just here to inform the user that a device is currently being shared; by default it's just an icon near the url bar, the popup isn't visible unless the user clicks the icon. > @@ +223,4 @@ > > info("requesting devices"); > > content.wrappedJSObject.requestDevice(true, true); > > }); > > + expectNotification("getUserMedia:request"); > > These seem new and decrement a global counter, can you explain why you added > them and/or how it's related to the random orange? Before the patch, I was waiting for this notification (yield promiseNotification("getUserMedia:request"... this actually decrements the counter once the notification is received) and then waiting for the popup notification. After the patch, I only wait for the popup notification, and then check after the fact that the observer service notification has been received as expected. The reason why I needed this change is that with the popupshown approach, I need to add an event listener before performing the action that will trigger the observer service notification, and the popup notification.
(In reply to Florian Quèze [:florian] [:flo] from comment #185) > Before the patch, I was waiting for this notification (yield > promiseNotification("getUserMedia:request"... this actually decrements the > counter once the notification is received) and then waiting for the popup > notification. How does it do that? The only code that I see touching gObservedTopics is the observer function and expectNotification. Neither got called from the code in the promiseNotification code that you removed. What am I missing?
(In reply to :Gijs Kruitbosch from comment #187) > (In reply to Florian Quèze [:florian] [:flo] from comment #185) > > Before the patch, I was waiting for this notification (yield > > promiseNotification("getUserMedia:request"... this actually decrements the > > counter once the notification is received) and then waiting for the popup > > notification. > > How does it do that? The only code that I see touching gObservedTopics is > the observer function and expectNotification. Neither got called from the > code in the promiseNotification code that you removed. What am I missing? Oh, there are 3 similarly named functions that are not closed to each other that do really different things... that is confusing. :-( Can you rename promiseNotification to promiseObserverCalled or something?
Attached patch popupshown approach, v2 (deleted) — — Splinter Review
(In reply to :Gijs Kruitbosch from comment #188) > Can you rename promiseNotification to promiseObserverCalled or something? Sure. To keep consistency I also renamed: expectNotification -> expectObserverCalled expectNoNotifications -> expectNoObserverCalled
Attachment #8386777 - Attachment is obsolete: true
Attachment #8387471 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8387471 [details] [diff] [review] popupshown approach, v2 Review of attachment 8387471 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_get_user_media.js @@ +82,3 @@ > > + PopupNotifications.panel.addEventListener("popupshown", function () { > + PopupNotifications.panel.removeEventListener("popupshown", arguments.callee); Nit: name the function and use a reference instead of arguments.callee, which is frowned upon
Attachment #8387471 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/51f11b099596
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Still happening :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 30 → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #200) > Still happening :( Indeed; comment 195 and comment 199 are with the third patch applied (the other comments are on try, so I'm not sure if that was with this patch, or try pushes based on older changesets). Both are on Linux. I wonder if the intermittent still happens on Mac.
I decided that comment 202 was likely related to this rather than filing a new bug (even though it was a timeout).
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #203) > I decided that comment 202 was likely related to this rather than filing a > new bug (even though it was a timeout). comment 194 is also a timeout (it was on try though). I'm not sure it's the same issue. The intermittent failure we've been trying to debug here was mostly related to the popup notification never showing up (likely due to focus issues). In both timeout logs, I see a chrome://browser/content/browser.xul window is garbage collected soon after the last line printed by browser_get_user_media.js. I wonder if the GC could be interfering with something the media manager does. Jesup, is this an idea worth exploring?
Flags: needinfo?(rjesup)
MediaManager certainly could be affected by GC's having happened, so probably yes it's worth exploring
Flags: needinfo?(rjesup)
The failure in comment 207 is a timeout too, but there's no browser.xul window getting GC'ed immediately after the "browser_get_user_media.js | requesting devices" line.
Summary: Intermittent | browser_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null → Intermittent browser_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out
(In reply to TBPL Robot from comment #211) > RyanVM > https://tbpl.mozilla.org/php/getParsedLog.php?id=35903767&tree=Mozilla- > Inbound > Rev3 Fedora 12 mozilla-inbound debug test mochitest-browser-chrome on > 2014-03-10 11:08:53 > revision: 4535980dba45 > slave: talos-r3-fed-094 > > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js | Test timed out > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_offlineQuotaNotification.js | uncaught exception - ReferenceError: > ok is not defined at > chrome://mochitests/content/browser/browser/base/content/test/general/ > browser_get_user_media.js:86 Ugh. So the popupshown handler isn't cleaned up either, which is causing the failures in the other tests. Florian, this is starting to look like there might actually be something wrong with the code that's meant to show these notifications... You're attaching the listener before activating the popups, and then the popups don't show, not even as far as the listener is aware. That's not good (unless the popup is already open from a previously run test or something?). :-\ Can you check if they're failing at a consistent place in the test? You open the same popup rather a lot (which might also mean that it might be good to split the test up).
Flags: needinfo?(florian)
(In reply to :Gijs Kruitbosch from comment #212) > Can you check if they're failing at a consistent place in the test? You open > the same popup rather a lot (which might also mean that it might be good to > split the test up). All the failures I've seen are on the very first getUserMedia call; the very beginning of the test. So I'm afraid splitting the test wouldn't help us at all here. If we still suspect the fullscreen test that's running before this one, maybe we could just get this one to run before the fullscreen test, to check if these 2 tests have bad interactions?
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #213) > If we still suspect the fullscreen test that's running before this one, > maybe we could just get this one to run before the fullscreen test, to check > if these 2 tests have bad interactions? I suspected it on mac, but a lot of the failures are on Linux. I don't know that there's anything wrong with the property there (but it could be that that's just something I don't know). I guess I can rs a name change to move it before the fullscreen test (that's what it'd take, AIUI we have checks that those tests are listed in alphabetical order). Please add a comment to the browser.ini file if you do this. I'd suggest using try to test this but it's rare enough that you'd need a lot of retriggers, so just pushing that seems like the simplest way to figure out if that helps or not. However, as I noted in an earlier comment, I would also like you to check if there really isn't a way that the test could be pointing to a real failure: the popupshown method not being called is very strange. One other thing that you could probably do is add an assert that the popup isn't *already shown* when the test starts waiting for the event. Likely a different failure mode than what we were seeing before, but that's the last explanation I can think of for "the test is wrong, the code is fine".
Something interesting/confusing here: we seem to have 2 significantly different ways in which this fails. Sometimes the "popupshown" event is never fired. And sometimes the event is fired, PopupNotifications.getNotification("webRTC-shareDevices") returns something, but PopupNotifications.panel.firstChild is null. The ideas we haven't fully explored yet: - a previous test may do something that interferes with focus. We suspect specifically the fullscreen test. The attached patch renames browser_get_user_media.js to browser_devices_get_user_media.js so that it executes before the fullscreen test. - there may be leftover notifications from a previous test. The attached patch adds a test for this at the very beginning of the test. If this is the problem, we should know it from the log of the next failures. I also added a check for PopupNotifications.isPanelOpen before the test for PopupNotifications.panel.firstChild. I think isPanelOpen is what we really want to test and panel.firstChild is just the implementation detail that we want to verify because the rest of the test depends on it. I pushed this to try (https://tbpl.mozilla.org/?tree=Try&rev=5820b885f3cf -- ignore the older changesets that I pushed by accident, they are unrelated to the tip changeset) and it's green; so the 2 added tests don't break things.
Attachment #8390583 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8390583 [details] [diff] [review] Rename, and check no previous popup notification Review of attachment 8390583 [details] [diff] [review]: ----------------------------------------------------------------- If this fixes the relevant type of orange, we should file a separate bug on the fullscreen test.
Attachment #8390583 - Flags: review?(gijskruitbosch+bugs) → review+
https://tbpl.mozilla.org/php/getParsedLog.php?id=36247117&tree=Fx-Team
Summary: Intermittent browser_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out → Intermittent browser_get_user_media.js, | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out
(In reply to Phil Ringnalda (:philor) from comment #268) > https://tbpl.mozilla.org/php/getParsedLog.php?id=36247117&tree=Fx-Team This failure is on a "Linux x64 ASAN" build; I think it's the first time it fails there. Other interesting things from this log: - There's no notification from previous tests (PopupNotifications._currentNotifications.length is 0 when starting the test). - We receive a popupshown event, but PopupNotifications.isPanelOpen is false. This points increasingly toward a PopupNotifications bug.
See if I can manage to properly paste this time. It's also in browser_devices_get_user_media.js, which also enjoys failing on the 3-chunk browser-chrome which is running hidden on mozilla-inbound. https://tbpl.mozilla.org/php/getParsedLog.php?id=36311073&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=36314149&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=36313987&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=36314299&tree=Mozilla-Inbound
Summary: Intermittent browser_get_user_media.js, | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out → Intermittent browser_get_user_media.js,browser_devices_get_user_media.js | Unexpected Exception: TypeError: PopupNotifications.panel.firstChild is null or Test timed out
(In reply to Phil Ringnalda (:philor) from comment #270) > https://tbpl.mozilla.org/php/getParsedLog.php?id=36311073&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=36314149&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=36313987&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=36314299&tree=Mozilla- > Inbound These are all Linux debug. Still curious to see if the test still fails on Mac. If it doesn't any more, then the fullscreen test was likely the cause of the failure on Mac, and we have a different cause for Linux.
(In reply to Florian Quèze [:florian] [:flo] from comment #271) > These are all Linux debug. > > Still curious to see if the test still fails on Mac. If it doesn't any more, > then the fullscreen test was likely the cause of the failure on Mac, and we > have a different cause for Linux. I wonder if bug 969608 could be related.
Personally, I'd look for related things for the Linux failures by searching for "skip-if = os == "linux"" (with all the possible spacing and quotes and the way that sometimes people realized they really meant toolkit == "gtk2" || toolkit == "gtk3"). Sometimes they'll have notification in the filename, or the bug summary, like bug 951680, other times they won't, but there's a long long trail of tears for notification tests on Linux.
10 days since the last check-in (renaming the test to run it before the fullscreen test), we haven't got a single Mac failure. We still get Linux Debug failures frequently, and got 1 ASAN opt failure. Would there be a way to disable the test for Linux debug, but keep it running on Linux opt?
(In reply to Florian Quèze [:florian] [:flo] from comment #305) > 10 days since the last check-in (renaming the test to run it before the > fullscreen test), we haven't got a single Mac failure. > > We still get Linux Debug failures frequently, and got 1 ASAN opt failure. > Would there be a way to disable the test for Linux debug, but keep it > running on Linux opt? Yes: https://developer.mozilla.org/en-US/docs/XPCshell_Test_Manifest_Expressions E.g. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser.ini#84
(In reply to Florian Quèze [:florian] [:flo] from comment #305) > 10 days since the last check-in (renaming the test to run it before the > fullscreen test), we haven't got a single Mac failure. Can you file a bug against the fullscreen test and CC me and mstange? See also https://bugzilla.mozilla.org/show_bug.cgi?id=946768#c2 . :-(
Attached patch Disable on Linux debug (deleted) — — Splinter Review
Per comment 279, comment 305 and comment 306, I think disabling the test on Linux Debug (and only there) is the best thing to do at this point.
Attachment #8395642 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8395642 [details] [diff] [review] Disable on Linux debug Review of attachment 8395642 [details] [diff] [review]: ----------------------------------------------------------------- rs=me In future, you don't normally need review on disabling tests (I mean, use your good judgment - if you're disabling 500 things at once, probably good to ask, but for something like this, you can just land it assuming you're sure about your use of the manifest syntax)
Attachment #8395642 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #307) > (In reply to Florian Quèze [:florian] [:flo] from comment #305) > > 10 days since the last check-in (renaming the test to run it before the > > fullscreen test), we haven't got a single Mac failure. > > Can you file a bug against the fullscreen test and CC me and mstange? See > also https://bugzilla.mozilla.org/show_bug.cgi?id=946768#c2 . :-( Filed bug 987115.
(In reply to :Gijs Kruitbosch from comment #309) > rs=me > > In future, you don't normally need review on disabling tests (I mean, use > your good judgment - if you're disabling 500 things at once, probably good > to ask, but for something like this, you can just land it assuming you're > sure about your use of the manifest syntax) Thanks, good to now! It's the first time I use this syntax, so I was reasonably confident, but not fully sure. Test disabled for Linux Debug: https://hg.mozilla.org/integration/fx-team/rev/6df8169d0716
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/6df8169d0716
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [test disabled on Linux debug]
Target Milestone: Firefox 31 → ---
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Status: REOPENED → RESOLVED
Closed: 11 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: