Closed Bug 464398 Opened 11 years ago Closed 9 years ago

HTML5 Media elements fire events while in bfcache

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: cajbir, Assigned: cajbir)

References

()

Details

Attachments

(2 files, 5 obsolete files)

<video> and <audio> can cause events to fire, and javascript attached to those events to run, while in bfcache. Steps to reproduce:

1. Visit page in URL
2. navigagte to another page, this putting page in (1) in bfcache
3. The progress events continue to fire for the page in bfcache.
   You can see this via the numerous warnings spewed to the debug console:

WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file content/base/src/nsContentUtils.cpp, line 2722
-1225009376[b6c10060]: WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject) failed: file content/base/src/nsContentUtils.cpp, line 4372
WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject) failed: file content/base/src/nsContentUtils.cpp, line 4372

In addition if the video was playing then events like ended, aborted, stalled, etc will all be raised.
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].
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
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?
Attachment #460784 - Flags: review?(roc)
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: --- → ?
Attachment #460784 - Flags: review?(roc)
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.
Attachment #460784 - Attachment is obsolete: true
Attachment #463053 - Flags: review?(roc)
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.
Depends on: 584615
As discussed with roc and cpearce in person I've raised bug 584615 to remove the progress events as per spec.
Attachment #463053 - Flags: review?(roc)
Modified to apply on top of bug 584615. It removes the need for keeping track of pending progress events.
Attachment #463053 - Attachment is obsolete: true
Attachment #463436 - Flags: review?(roc)
I think nsTArray<nsString> is better than nsStringArray.
Change from nsStringArray to using nsTArray<nsString>
Attachment #463436 - Attachment is obsolete: true
Attachment #464255 - Flags: review?(roc)
Attachment #463436 - Flags: review?(roc)
+    nsString s = mPendingEvents[i];
+    DispatchAsyncEvent(s);

It looks like you're copying the string here for no reason.
Address review comment
Attachment #464255 - Attachment is obsolete: true
Attachment #464274 - Flags: review?(roc)
Attachment #464255 - Flags: review?(roc)
Rebase on top of bugs it depends on and fixes bitrot. carried r+ forward.
Attachment #464274 - Attachment is obsolete: true
Attachment #473931 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/963d95181b4b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
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 → ---
http://hg.mozilla.org/mozilla-central/rev/33f5bd479735
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 593748
You need to log in before you can comment on or make changes to this bug.