464398 - HTML5 Media elements fire events while in bfcache

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

<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.

Comment 1

[Matthew Cline]

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].

Comment 2

[Matthew Cline]

Attachment: stack trace and gdb dump of nsDocument instance

Comment 3

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

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?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[cajbir (:cajbir)]

Should this be blocking 2.0 then? Currently as well as the warning events are dropped on the floor and scripts will miss them.

Comment 6

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This dispatches pending simple events before pending progress events. Is that correct? Maybe we should keep the events in the same relative order.

Comment 8

[cajbir (:cajbir)]

Or we could follow the spec and not have progress events at all - that'd fix the issue and remove duplication.

Comment 9

[cajbir (:cajbir)]

As discussed with roc and cpearce in person I've raised bug 584615 to remove the progress events as per spec.

Comment 10

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

Modified to apply on top of bug 584615. It removes the need for keeping track of pending progress events.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think nsTArray<nsString> is better than nsStringArray.

Comment 12

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

Change from nsStringArray to using nsTArray<nsString>

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    nsString s = mPendingEvents[i];
+    DispatchAsyncEvent(s);

It looks like you're copying the string here for no reason.

Comment 14

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

Address review comment

Comment 15

[cajbir (:cajbir)]

Attachment: Don't process media events while in bfcache

Rebase on top of bugs it depends on and fixes bitrot. carried r+ forward.

Comment 16

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/963d95181b4b

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/33f5bd479735