Closed Bug 1227320 Opened 10 years ago Closed 10 years ago

Intermittent e10s browser_audioTabIcon.js | Expected the correct soundplaying attribute on the new tab - Got false, expected true

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Version:
45 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
e10s + ---
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: philor, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Need to wait two TabAttrModified events to ensure soundplaying attribue is updated v2
6.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Keywords: intermittent-failure
Priority: -- → P5
What component should this be, ehsan?
Flags: needinfo?(ehsan)
This is the component from which baku broke it, in bug 1213154, so this seems like the appropriate one.
Summary: Intermittent e10s /browser_audioTabIcon.js | Expected the correct soundplaying attribute on the new tab - Got false, expected true → Intermittent e10s browser_audioTabIcon.js | Expected the correct soundplaying attribute on the new tab - Got false, expected true
Andrea, can you please have a look? Thanks!
Flags: needinfo?(ehsan) → needinfo?(amarchesini)
The bustage is from bfcache changes. This is DOM.
Component: Audio/Video: Playback → DOM
(In reply to Ben Kelly [:bkelly] from comment #15) > The bustage is from bfcache changes. This is DOM. Am I remembering correctly that you made some changes in this area recently, smaug?
Flags: needinfo?(bugs)
I don't recall any changes to bfcache recently, and nsDocShell.cpp or nsSHEntry.cpp log don't hint any either. Perhaps bkelly meant audio's bfcache handling.
Flags: needinfo?(bugs)
baku, wdyt?
Yea, I said bfcache based on the suspected cause in bug 1213154 which was something baku worked on.
swapBrowsersAndCloseOther involes nsDocument::OnPageHide and nsDocument::OnPageShow. Each function leads to active/inactive 'audio-playback' notifications respectively. 'audio-playback' notification is asynchronously processed, and the 'audio-playback' notification is also passed to chrome process asynchronously at https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/toolkit/content/browser-content.js#733 So if 100ms timeout happens between inactive and active notifications, 'soundplaying' attribute will not set. I guess it's the failure case. This patch avoids the case to receive two TabAttrModified events corresponding to those inactive/active notifications. This patch also splits test_swapped_browser into two separated test functions, both functions have redundant parts but I think it is easier to understand. This patch also uses 'audiochannel-activity-normal' instead of 'audio-playback' because 'audio-playback' notification is not passed to chrome process on E10S. A try result with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=410b74c6c2f0&selectedJob=14871020
Attachment #8701876 - Flags: feedback?(ehsan)
Attachment #8701876 - Flags: feedback?(amarchesini)
Comment on attachment 8701876 [details] [diff] [review] Need to wait two TabAttrModified events to ensure soundplaying attribue is updated Review of attachment 8701876 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_audioTabIcon.js @@ +196,5 @@ > + > + ok(newTab.hasAttribute("muted"), "Expected the correct muted attribute on the new tab"); > + ok(newTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the new tab"); > + > + let nReceivedSoundPlaying = 0; Nit: please drop the n prefix.
Attachment #8701876 - Flags: feedback?(ehsan) → review+
Thank you, Ehsan! Would you please review too?
Assignee: nobody → hiikezoe
Attachment #8701876 - Attachment is obsolete: true
Attachment #8701876 - Flags: feedback?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8702744 - Flags: review?(ehsan)
Comment on attachment 8702744 [details] [diff] [review] Need to wait two TabAttrModified events to ensure soundplaying attribue is updated v2 Review of attachment 8702744 [details] [diff] [review]: ----------------------------------------------------------------- I already r+ed. :-) Thanks for the fix!
Attachment #8702744 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/6f4da397ac3c
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: