Closed Bug 1453971 Opened 5 years ago Closed 5 years ago

Assertion failure: !nsContentUtils::IsInStableOrMetaStableState() in media-source/mediasource-remove.html test with GC zeal

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: jonco, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Found running tests with GC zeal mode 8, 5000.  The stack shows that nsAutoScriptBlocker can attempt to dispatch a DOM event during cycle collection.

I couldn't reproduce this locally running just this test.

https://treeherder.mozilla.org/logviewer.html#?job_id=173482170&repo=try&lineNumber=15380

[task 2018-04-13T11:56:16.327Z] 11:56:16     INFO - PID 6359 | Assertion failure: !nsContentUtils::IsInStableOrMetaStableState(), at /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:682
[task 2018-04-13T11:56:16.444Z] 11:56:16     INFO - PID 6359 | #01: mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.448Z] 11:56:16     INFO - PID 6359 | #02: nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.456Z] 11:56:16     INFO - PID 6359 | #03: mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.464Z] 11:56:16     INFO - PID 6359 | #04: mozilla::AsyncEventDispatcher::Run() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.470Z] 11:56:16     INFO - PID 6359 | #05: nsContentUtils::RemoveScriptBlocker() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.478Z] 11:56:16     INFO - PID 6359 | #06: nsAutoScriptBlocker::~nsAutoScriptBlocker() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.486Z] 11:56:16     INFO - PID 6359 | #07: nsDocument::cycleCollection::Unlink(void*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.494Z] 11:56:16     INFO - PID 6359 | #08: nsHTMLDocument::cycleCollection::Unlink(void*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.496Z] 11:56:16     INFO - PID 6359 | #09: nsCycleCollector::CollectWhite() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.505Z] 11:56:16     INFO - PID 6359 | #10: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.509Z] 11:56:16     INFO - PID 6359 | #11: nsCycleCollector::FinishAnyCurrentCollection() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.518Z] 11:56:16     INFO - PID 6359 | #12: mozilla::CycleCollectedJSRuntime::OnGC(JSContext*, JSGCStatus) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.522Z] 11:56:16     INFO - PID 6359 | #13: js::gc::GCRuntime::maybeCallGCCallback(JSGCStatus) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.532Z] 11:56:16     INFO - PID 6359 | #14: js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.534Z] 11:56:16     INFO - PID 6359 | #15: js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.542Z] 11:56:16     INFO - PID 6359 | #16: js::gc::GCRuntime::runDebugGC() (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.550Z] 11:56:16     INFO - PID 6359 | #17: js::gc::GCRuntime::gcIfNeededAtAllocation(JSContext*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.554Z] 11:56:16     INFO - PID 6359 | #18: bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.562Z] 11:56:16     INFO - PID 6359 | #19: JSObject* js::Allocate<JSObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.571Z] 11:56:16     INFO - PID 6359 | #20: js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.575Z] 11:56:16     INFO - PID 6359 | #21: js::LexicalEnvironmentObject::createTemplateObject(JSContext*, JS::Handle<js::Shape*>, JS::Handle<JSObject*>, js::gc::InitialHeap) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.579Z] 11:56:16     INFO - PID 6359 | #22: js::LexicalEnvironmentObject::create(JSContext*, JS::Handle<js::LexicalScope*>, JS::Handle<JSObject*>, js::gc::InitialHeap) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2018-04-13T11:56:16.587Z] 11:56:16     INFO - PID 6359 | #23: js::LexicalEnvironmentObject::create(JSContext*, JS::Handle<js::LexicalScope*>, js::AbstractFramePtr) (/builds/worker/workspace/build/application/firefox/libxul.so)
But where are we in stable state? We shouldn't be running JS at that time.
Would need better stack trace.
Component: DOM → General
Attached file stack_trace.txt
Here's the full stack trace.
URLMainThread::CreateObjectURL is doing something bad.
or, hmm, RunnableFunction<mozilla::dom::URLMainThread::CreateObjectURL
URLMainThread::CreateObjectURL is doing something bad.
Blocks: 1422314
Component: General → DOM
The point is that bug 1422314 added code which may trigger JS to run at time when it shouldn't run.
Or that bug nsIAsyncShutdownService shouldn't be implemented in JS.
s/bug/interface/
Andrea, could you take a look at this? It sounds like it may be a regression from your patch.
Flags: needinfo?(amarchesini)
Keywords: regression
Priority: -- → P2
Attached patch stable.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8974661 - Flags: review?(bugs)
Attached patch stable.patch (obsolete) — Splinter Review
Attachment #8974661 - Attachment is obsolete: true
Attachment #8974661 - Flags: review?(bugs)
Attachment #8974664 - Flags: review?(karlt)
Comment on attachment 8974664 [details] [diff] [review]
stable.patch

>Bug 1453971 - URL.createObjectURL(MediaSource) deoesn't require a stable state, r?karlt

>   nsCOMPtr<nsIRunnable> revocation =
>     NS_NewRunnableFunction("dom::URLMainThread::CreateObjectURL", [url] {
>       nsHostObjectProtocolHandler::RemoveDataEntry(url);
>     });
> 
>-  nsContentUtils::RunInStableState(revocation.forget());
>-
>   CopyASCIItoUTF16(url, aResult);
> }

I don't understand.  How will |revocation| run in stable state?
Attachment #8974664 - Flags: review?(karlt)
Attached patch stable.patch (obsolete) — Splinter Review
Attachment #8974664 - Attachment is obsolete: true
Attachment #8975348 - Flags: review?(karlt)
Comment on attachment 8975348 [details] [diff] [review]
stable.patch

>Bug 1453971 - URL.createObjectURL(MediaSource) deoesn't require a stable state, r?karlt

Can you explain why a stable state is not required, please?

The intention is to revoke before javascript can run when the next event is processed, but I assume this change would permit pending events to run before the url is revoked.
Attachment #8975348 - Flags: review?(karlt)
Attached patch stable.patchSplinter Review
I double checked the spec and there is nothing special in CreateObjectURL for MediaSource. It should behave exactly how the other createObjectURL(blob).
For blobURLs, we register them in the global object in order to remove them when deleted.
This patch applies the same logic here.
Attachment #8975348 - Attachment is obsolete: true
Attachment #8979011 - Flags: review?(karlt)
Comment on attachment 8979011 [details] [diff] [review]
stable.patch

(In reply to Andrea Marchesini [:baku] from comment #15)
> I double checked the spec and there is nothing special in CreateObjectURL
> for MediaSource. It should behave exactly how the other
> createObjectURL(blob).

This is a bug that was introduced into the spec, due to a bug in the Chrome
implementation.  It is tracked by
https://github.com/w3c/media-source/issues/156

The buggy behavior of Chrome is not useful, and so can only cause problems
with leaked resources, some of which can be very large.  I don't want to
provide that behavior.
Attachment #8979011 - Flags: review?(karlt) → review-
What is going wrong here?

Why is script being run from nsDocument::cycleCollection::Unlink()?

https://hg.mozilla.org/try/annotate/797d737aea18669397474e04c80cbeb78b196b63/dom/base/nsDocument.cpp#l1981
Using nsContentUtils::RunInStableState is wrong. It means that a URL created, and used with a setTimeout() will not work. Am I right? Here a simple test:

    <script>
var mediaSource = new MediaSource;
var url = URL.createObjectURL(mediaSource);
setTimeout(() => {
  var video = document.createElement('video');
  document.body.appendChild(video);
  video.src = url;
  ... fetch + mediaSource.addSourceBuffer(...) + video.play();
}, 10000);
Flags: needinfo?(karlt)
(In reply to Andrea Marchesini [:baku] from comment #18)
> Using nsContentUtils::RunInStableState is wrong. It means that a URL
> created, and used with a setTimeout() will not work. Am I right?

A URL created before a setTimeout cannot be successfully used in the
setTimeout callback.  That is the intended behavior, and the behavior tested
by web platform tests.

createObjectURL is provided merely as as a mechanism to connect a MediaSource
to HTMLMediaElement.src.  HTMLMediaElement.srcObject would have been a much
better solution, but Firefox at least does not yet support that.

When using MediaSource in a callback, the createObjectURL() call would be
performed within the setTimeout callback.
Flags: needinfo?(karlt)
So the current spec doesn't warrant our behavior - even if our behavior makes sense.
If other browsers don't have auto revoke, it is a bit hard to argue why we should. Some pages might after all work now in other engines, but not in Gecko.

But this bug could be fixed also differently by not triggering JS to run in stable state (and leave auto revoke code in place).
Attached patch stable2.patchSplinter Review
I filed a separate bug to discuss auto-revoke in URL.createObjectURL(mediaSource): bug 1463422.
Attachment #8979541 - Flags: review?(bugs)
Comment on attachment 8979541 [details] [diff] [review]
stable2.patch

>   Create(const nsACString& aURI, bool aBroadcastToOtherProcesses)
>   {
>     MOZ_ASSERT(NS_IsMainThread());
> 
>     RefPtr<ReleasingTimerHolder> holder =
>       new ReleasingTimerHolder(aURI, aBroadcastToOtherProcesses);
> 
>-    auto raii = mozilla::MakeScopeExit([holder] {
>-      holder->CancelTimerAndRevokeURI();
>+    SystemGroup::EventTargetFor(TaskCategory::Other)->Dispatch(holder.forget());
>+  }
Shouldn't you leave raii here, since Dispatch may fail.
release raii in case Dispatch succeeds


> 
>-  // nsINamed interface
>+  // nsIAsyncShutdownBlocker interface
> 
>-  NS_IMETHOD
>-  GetName(nsACString& aName) override
>-  {
>-    aName.AssignLiteral("ReleasingTimerHolder");
>-    return NS_OK;
>-  }
>-
>-  // nsIAsyncShutdownBlocker interface
>+  using Runnable::GetName;
This is for nsINamed

> 
>   NS_IMETHOD
>   GetName(nsAString& aName) override
This is for nsIAsyncShutdownBlocker
Attachment #8979541 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03813b3e4edd
ReleasingTimerHolder::Create should use a timer to avoid nsIAsyncShutdownBlocker (implemented in JS) to run during stable state, r=smaug
https://hg.mozilla.org/mozilla-central/rev/03813b3e4edd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1463767
AFAICT, this can ride the trains, but please correct me if I'm wrong.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.