Last Comment Bug 593305 - "ASSERTION: Load event should not be delayed at start of resource selection"
: "ASSERTION: Load event should not be delayed at start of resource selection"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 343943 485288
  Show dependency treegraph
 
Reported: 2010-09-03 01:43 PDT by Jesse Ruderman
Modified: 2011-04-19 18:20 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (390 bytes, text/html)
2010-09-03 01:43 PDT, Jesse Ruderman
no flags Details
Patch: Remove assertion (1.17 KB, patch)
2011-04-19 16:37 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-09-03 01:43:01 PDT
Created attachment 471787 [details]
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] 2010-09-03 02:12:39 PDT
Probably new from bug 485288 landing?
Comment 2 Chris Pearce (:cpearce) 2010-09-03 02:23:23 PDT
(In reply to comment #1)
> Probably new from bug 485288 landing?

Definitely. That didn't take long!
Comment 3 Chris Pearce (:cpearce) 2011-04-19 16:34:32 PDT
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) 2011-04-19 16:37:27 PDT
Created attachment 527149 [details] [diff] [review]
Patch: Remove assertion
Comment 5 Chris Pearce (:cpearce) 2011-04-19 18:20:10 PDT
http://hg.mozilla.org/mozilla-central/rev/298cbc884044

Note You need to log in before you can comment on or make changes to this bug.