1548992 - Assertion failure: mDestroyCalled, at src/dom/base/nsFrameLoader.cpp:200

Status: RESOLVED DUPLICATE of bug 1458106

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: mDestroyCalled, at /builds/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:222

#0 nsFrameLoader::~nsFrameLoader() /src/dom/base/nsFrameLoader.cpp:222:3
#1 nsFrameLoader::~nsFrameLoader() /src/dom/base/nsFrameLoader.cpp:218:33
#2 SnowWhiteKiller::~SnowWhiteKiller() /src/xpcom/base/nsCycleCollector.cpp:2416:7
#3 nsCycleCollector::FreeSnowWhite(bool) /src/xpcom/base/nsCycleCollector.cpp:2607:3
#4 nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /src/xpcom/base/nsCycleCollector.cpp:3582:3
#5 nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /src/xpcom/base/nsCycleCollector.cpp:3411:9
#6 nsCycleCollector_collect(nsICycleCollectorListener*) /src/xpcom/base/nsCycleCollector.cpp:3947:21
#7 nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /src/dom/base/nsJSEnvironment.cpp:1432:3
#8 mozilla::dom::FuzzingFunctions_Binding::cycleCollect(JSContext*, unsigned int, JS::Value*) /src/obj-firefox/dom/bindings/FuzzingFunctionsBinding.cpp:66:3
#9 CallJSNative /src/js/src/vm/Interpreter.cpp:443:13
#10 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:535
#11 CallFromStack /src/js/src/vm/Interpreter.cpp:594:10
#12 Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3082
#13 js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:423:10
#14 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:563:13
#15 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:606:8
#16 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2647:10
#17 mozilla::dom::IdleRequestCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::IdleDeadline&, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/WindowBinding.cpp:1047:8
#18 Call /src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:780:12
#19 Call /src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:793
#20 mozilla::dom::IdleRequest::IdleRun(nsPIDOMWindowInner*, double, bool) /src/dom/base/IdleRequest.cpp:63
#21 nsGlobalWindowInner::RunIdleRequest(mozilla::dom::IdleRequest*, double, bool) /src/dom/base/nsGlobalWindowInner.cpp:673:12
#22 nsGlobalWindowInner::ExecuteIdleRequest(mozilla::TimeStamp) /src/dom/base/nsGlobalWindowInner.cpp:701:3
#23 IdleRequestExecutor::Run() /src/dom/base/nsGlobalWindowInner.cpp:543:13
#24 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1180:14
#25 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
#26 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:88:21
#27 RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
#28 RunHandler /src/ipc/chromium/src/base/message_loop.cc:308
#29 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290
#30 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:137:27
#31 XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:919:20
#32 RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
#33 RunHandler /src/ipc/chromium/src/base/message_loop.cc:308
#34 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290
#35 XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:757:34
#36 content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#37 main /src/browser/app/nsBrowserApp.cpp:263

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The destructor of nsFrameLoader expects that its Destroy() has already been called before deleted. However, if it's deleted by cycle collector, it won't be called unless each Unlink() whose class owns frame loader calls it. So, this must be simple mistake, but it hits MOZ_RELEASE_ASSERT(). So, we should fix this ASAP.

Comment 2

[Andreas Farre [:farre]]

I tried looking into this, but the testcase doesn't reproduce. How frequent should I expect it to assert?

Comment 3

[Andreas Farre [:farre]]

Ok, I'm blindly guessing here, but could this be the reason:

The thing that owns the nsFrameLoader is (naturally) nsFrameLoaderOwner. If we look to who inherits from nsFrameLoaderOwner we find three classes, nsObjectLoadingContent, nsGenericHTMLFrameElement, and XULFrameElement[1]. Of these three nsGenericHTMLFrameElement and XULFrameElement calls nsFrameLoader::Destroy while unlinking[2, 3]. nsObjectLoadingContent is inherited by HTMLObjectElement[4] and HTMLEmbedElement[5] and neither destroys the nsFrameLoader in the nsObjectLoadingContent base. HTMLObjectElement does unlinking, HTMLEmbedElement does not. Extending HTMLObjectElement and HTMLEmbedElement to also destroy nsFrameLoader when unlinking could potentially be what's missing.

Looking at nsGenericHTMLFrameElement it turns out :baku added exactly that, so adding him for a ni if this seems like a good plan.

I'll implement this and kick off a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e14371570ca34edfba5c444dcbe00f92f5edde

[1] https://searchfox.org/mozilla-central/search?q=public+nsFrameLoaderOwner&path=
[2] https://searchfox.org/mozilla-central/source/dom/html/nsGenericHTMLFrameElement.cpp#42-44
[3] https://searchfox.org/mozilla-central/source/dom/xul/XULFrameElement.cpp#27-29
[4] https://searchfox.org/mozilla-central/source/dom/html/HTMLObjectElement.h
[5] https://searchfox.org/mozilla-central/source/dom/html/HTMLEmbedElement.h

Comment 4

[Tyson Smith [:tsmith]]

Attachment: testcase.html

The original test case became less reliable in the last 2 days not sure why. I have updated it and it should work within a few seconds now.

Be sure to set dom.disable_open_during_load=false

Comment 5

[Andrea Marchesini [:baku]]

Looking at nsGenericHTMLFrameElement it turns out :baku added exactly that, so adding him for a ni if this seems like a good plan.

I agree with your comment. Nothing else to say :)

Comment 6

[Andreas Farre [:farre]]

The root cause for this is that the neither of the following assert doesn't hold:

void nsObjectLoadingContent::UnloadObject(bool aResetState) {
  CancelImageRequests(false);
  if (mFrameLoader) {
    RefPtr<nsFrameLoader> loader = mFrameLoader;
    mFrameLoader->Destroy();
    MOZ_DIAGNOSTIC_ASSERT(mFrameLoader->IsDead());
    MOZ_DIAGNOSTIC_ASSERT(loader == mFrameLoader);
    mFrameLoader = nullptr;
  }

because LoadObject is re-entrant. So unfortunately it doesn't happen because of the missing unlinks for nsObjectLoadingContent (even though that should probably be added as well).

Comment 7

[Andreas Farre [:farre]]

Or rather, it's not that LoadObject is re-entrant, it's that we can enter LoadObject with a Destroy higher up on the stack. This means that the pattern:

  mFrameLoader->Destroy();
  mFrameLoader = nullptr;

isn't valid, because nsFrameLoder::Destroy can have side effects that change the value of mFrameLoader!

Comment 8

[Neha Kochar [:neha]]

Andreas, is this a dup of bug 1458106?