Closed Bug 1548992 Opened 11 months ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 1458106
Tracking Status
firefox68 --- affected

People

(Reporter: tsmith, Assigned: farre)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Attachments

(1 file, 1 obsolete file)

Attached file testcase.html (obsolete) —

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
Flags: in-testsuite?

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.

Flags: needinfo?(kyle)
Flags: needinfo?(afarre)
Priority: -- → P1

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

Flags: needinfo?(twsmith)

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

Flags: needinfo?(afarre) → needinfo?(amarchesini)
Attached file 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

Attachment #9062661 - Attachment is obsolete: true
Flags: needinfo?(twsmith)
Assignee: nobody → afarre
Status: NEW → ASSIGNED
Flags: needinfo?(kyle)

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 :)

Flags: needinfo?(amarchesini)

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

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!

Andreas, is this a dup of bug 1458106?

Flags: needinfo?(afarre)
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Flags: needinfo?(afarre)
Resolution: --- → DUPLICATE
Duplicate of bug: 1458106
You need to log in before you can comment on or make changes to this bug.