593305 - "ASSERTION: Load event should not be delayed at start of resource selection"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Load event should not be delayed at start of resource selection.: '!mDelayingLoadEvent', file content/html/content/src/nsHTMLMediaElement.cpp, line 636

nsHTMLMediaElement::SelectResource [content/html/content/src/nsHTMLMediaElement.cpp:637]
nsSyncSection::Run [content/html/content/src/nsHTMLMediaElement.cpp:561]
nsBaseAppShell::RunSyncSections [widget/src/xpwidgets/nsBaseAppShell.cpp:349]
nsBaseAppShell::AfterProcessNextEvent [widget/src/xpwidgets/nsBaseAppShell.cpp:362]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:559]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:110]
MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:220]
MessageLoop::RunHandler [ipc/chromium/src/base/message_loop.cc:203]
MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:176]
nsBaseAppShell::Run [widget/src/xpwidgets/nsBaseAppShell.cpp:186]
nsAppStartup::Run [toolkit/components/startup/src/nsAppStartup.cpp:191]
XRE_main [toolkit/xre/nsAppRunner.cpp:3665]
main [browser/app/nsBrowserApp.cpp:158]
libc.so.6 + 0x1ec4d
firefox-bin + 0xd19

###!!! ASSERTION: Load event not delayed during source selection?: 'mDelayingLoadEvent', file content/html/content/src/nsHTMLMediaElement.cpp, line 530

nsHTMLMediaElement::NoSupportedMediaSourceError [content/html/content/src/nsHTMLMediaElement.cpp:530]
nsHTMLMediaElement::NotifyLoadError [content/html/content/src/nsHTMLMediaElement.cpp:690]
nsHTMLMediaElement::MediaLoadListener::OnStartRequest [content/html/content/src/nsHTMLMediaElement.cpp:302]
nsJSChannel::NotifyListener [dom/src/jsurl/nsJSProtocolHandler.cpp:855]
nsJSChannel::EvaluateScript [dom/src/jsurl/nsJSProtocolHandler.cpp:777]
nsRunnableMethodImpl<void (nsJSChannel::*)(), true>::Run [nsThreadUtils.h:348]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:110]
MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:220]
MessageLoop::RunHandler [ipc/chromium/src/base/message_loop.cc:203]
MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:176]
nsBaseAppShell::Run [widget/src/xpwidgets/nsBaseAppShell.cpp:186]
nsAppStartup::Run [toolkit/components/startup/src/nsAppStartup.cpp:191]
XRE_main [toolkit/xre/nsAppRunner.cpp:3665]
main [browser/app/nsBrowserApp.cpp:158]
libc.so.6 + 0x1ec4d
firefox-bin + 0xd19

Comment 1

[Matthew Gregan [:kinetik]]

Probably new from bug 485288 landing?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #1)
> Probably new from bug 485288 landing?

Definitely. That didn't take long!

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

My patch in bug 650994 causes this assertion to fire a lot in crashtest content/media/test/crashtests/459439-1.html, so I investigated this...

What happens there (and in Jesse's testcase here) is:
1. In an event we start a load, and then set a timeout to start a new load. Starting a new load queues up a "synchronous section", which will run immediately after we return to the main thread event loop (it jumps to the front of the event queue), before the timeout runs.
2. We return to the event loop, run nsHTMLMediaElement::SelectResource() in the sync section, which sets mDelayingLoadEvent=PR_TRUE, and triggers a load, then returns.
3. The timer runs before the load has a chance to progress, and triggers another load and queues up SelectResource() to run in a sync section. We normally set mDelayingLoadEvent to PR_FALSE when we load metadata, but we didn't get a chance to in this case.
4. SelectResource() runs and asserts that mDelayingLoadEvent==PR_FALSE, but it isn't because the previous load didn't fail or reach metadataloaded, so the assertion fires.
5. When the metadata loading finishes for the second load, we set mDelayingLoadEvent to PR_FALSE.

I think the assertion is wrong, if we start a new resource load before the old resource load has stopped delaying the load event, we should still delay the load event until the new resource load stops delaying the load event. We should remove the assertion.

FWIW in the spec's load algorithm description, it is specified that media element has "a delaying-the-load-event flag, which must begin in the false state", but doesn't explicitly state that the delaying the load event flag is reset when a previous load is aborted.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Remove assertion

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/298cbc884044