Closed Bug 464398 Opened 11 years ago Closed 9 years ago
HTML5 Media elements fire events while in bfcache
Building from code pulled from mercurial on April 12, I got the same warnings, but only when closing a tab, not when putting a tab into the background. I had to open and shut the tab for the same URL multiple times (different ads get loaded each time, so I kept trying), and the warning only happened when I closed the window while it was loading. I'll upload a stack trace of where the "WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject)" was generated, plus the gdb output of the nsDocument object of frame #9 [nsDocument::DispatchEvent].
Checks if the we are in the bfcache using the media elements existing flag for paused while inactive. If this is the case no events are raised. Without this patch an NS_WARNING is logged when events are raised when visiting this page and then navigating to about:blank: http://double.co.nz/video_test/suspend.html With this patch no warnings occur. I'm unsure how to write a test for this. Any advice?
I don't think dropping all of these events on the floor is correct. At least some of them should be queued so that if the page comes out of bfcache, scripts don't miss events.
Should this be blocking 2.0 then? Currently as well as the warning events are dropped on the floor and scripts will miss them.
blocking2.0: --- → ?
blocking2.0: ? → final+
Instead of discarding events I now save them in an array and dispatch them when restoring from the bfcache. I haven't done logic to detect and discard duplicate events as the number of events are usually in the order of 1 event of a given type and there didn't seem to be much point in extra logic to check for specific types of events and whether they are duplicates. Note that if a page contains video elements being loaded over http then that page will not go into bfcache while the video is loading/playing - see bug 584570.
This dispatches pending simple events before pending progress events. Is that correct? Maybe we should keep the events in the same relative order.
Or we could follow the spec and not have progress events at all - that'd fix the issue and remove duplication.
As discussed with roc and cpearce in person I've raised bug 584615 to remove the progress events as per spec.
Modified to apply on top of bug 584615. It removes the need for keeping track of pending progress events.
I think nsTArray<nsString> is better than nsStringArray.
Change from nsStringArray to using nsTArray<nsString>
+ nsString s = mPendingEvents[i]; + DispatchAsyncEvent(s); It looks like you're copying the string here for no reason.
Address review comment
Attachment #464274 - Flags: review?(roc) → review+
Rebase on top of bugs it depends on and fixes bitrot. carried r+ forward.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
backed out due to suspect increased orange in test_videocontrols_direction http://hg.mozilla.org/mozilla-central/rev/ebf786d2bcd5 http://hg.mozilla.org/mozilla-central/rev/b49a60a88ef2 see bug 595463.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.