Closed Bug 1227320 Opened 4 years ago Closed 4 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)

45 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla46
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)

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: 4 years ago
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.