1615480 - Crash due to SetSandboxFlags on BrowsingContext when !EverAttached()

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Andrew McCreight [:mccr8]]

This bug is for crash report bp-a0f6e953-f8d9-47f5-81c9-257130200213.

Top 10 frames of crashing thread:

0 XUL mozilla::ipc::IPDLParamTraits<mozilla::dom::BrowsingContext*>::Write docshell/base/BrowsingContext.cpp:1508
1 XUL mozilla::dom::PContentChild::SendCommitBrowsingContextTransaction ipc/ipdl/PContentChild.cpp:7306
2 XUL mozilla::dom::syncedcontext::Transaction<mozilla::dom::BrowsingContext>::Commit docshell/base/SyncedContextInlines.h:41
3 XUL void mozilla::dom::BrowsingContext::SetSandboxFlags<unsigned int&> docshell/base/BrowsingContext.h:128
4 XUL nsFrameLoader::ApplySandboxFlags dom/base/nsFrameLoader.cpp:3082
5 XUL mozilla::dom::HTMLIFrameElement::AfterSetAttr dom/html/HTMLIFrameElement.cpp:170
6 XUL mozilla::dom::Element::SetAttrAndNotify dom/base/Element.cpp:2353
7 XUL mozilla::dom::Element::SetAttr dom/base/Element.cpp:2213
8 XUL mozilla::dom::Element::SetAttribute dom/base/Element.cpp:1280
9 XUL mozilla::dom::Element_Binding::setAttribute dom/bindings/ElementBinding.cpp:1305

Crash reason is MOZ_DIAGNOSTIC_ASSERT(!aParam || aParam->EverAttached()).

Bug 1612894 landed to fix this, but it didn't go away, so I'm filing this new bug. Maybe it will be fixed by

Comment 1

[Andrew McCreight [:mccr8]]

Kris says that this is a different issue than bug 1612894, despite having the same crash signature.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Bug 1615403 is marked as blocking this, but that patch won't fix this particular issue. The assertion which is failing here will still fire when sending a MaybeDetached<BrowsingContext>. This issue is caused by trying to send a BrowsingContext before it is ever initialized, rather than after it has been discarded (hence the EverAttached).

This is a new state which was added by :kmag in bug 1582832. A BrowsingContext is created for a nsFrameLoader before the nsFrameLoader has been initialized, and it isn't attached into the tree until nsFrameLoader initialization. This stack implies that iframe.setAttribute is being called from script for a nsFrameLoader before the nsFrameLoader has had a chance to initialize.

Usually, a nsFrameLoader is initialized within a script runner, however other tasks could be queued to run in the script runner, and could trigger JS running before the nsFrameLoader has had a chance to be fully initialized. From looking at the stack for the linked crash report, that appears to be the case here.

nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&)
mozilla::dom::Document::EndUpdate()
nsContentUtils::RemoveScriptBlocker()
mozilla::AsyncEventDispatcher::Run()

Usually this wouldn't be an issue, as calling GetBrowsingContext() on the nsFrameLoader would force initialization by calling EnsureRemoteBrowser() or MaybeCreateDocShell(), but the particular code for ApplySandboxFlags instead directly uses mBrowsingContext, which does not force initialization.

It might be worthwhile to change the name of the mBrowsingContext field to discourage its direct use, and instead encourage using the GetBrowsingContext and GetExtantBrowsingContext helpers.

Comment 3

[Andrew McCreight [:mccr8]]

I'll mark this as a regression from bug 1582832. Feel free to remove that it if it is inaccurate.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

This was actually a regression from bug 1587062, not from bug 1582832.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1615480 - Part 1: Return a raw pointer from Get[Extant]BrowsingContext,

The BrowsingContext is guaranteed to be being kept alive by
nsFrameLoader::mBrowsingContext and by the nsDocShell or RemoteBrowser object.
This improves the ergonomics of this helper method, which may help avoid misuse
of mBrowsingContext.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1615480 - Part 2: Call GetBrowsingContext from ApplySandboxFlags,

This will ensure that the BrowsingContext has been attached before trying to
access properties on it.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1615480 - Part 3: Rename nsFrameLoader::mBrowsingContext to avoid confusion,

The new name should make it more clear that this should not be the default way
to look up the BrowsingContext instance from a nsFrameLoader, as it does not
ensure that the BrowsingContext has been fully initialized and attached to the
tree.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70aff2593d98
Part 1: Return a raw pointer from Get[Extant]BrowsingContext, r=kmag
https://hg.mozilla.org/integration/autoland/rev/ad31eae54af2
Part 2: Call GetBrowsingContext from ApplySandboxFlags, r=kmag
https://hg.mozilla.org/integration/autoland/rev/d6fd08e3fccf
Part 3: Rename nsFrameLoader::mBrowsingContext to avoid confusion, r=kmag

Comment 9

[Cosmin Sabou [:CosminS]]

Backed out 3 changesets (Bug 1615480) for causing build bustages on nsFrameLoader.cpp.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=d6fd08e3fccf60f68dfc7144845ccbd1d6a86edf&selectedJob=289953832

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=289953832&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/b06b79af757820d7853aed6ebabfff09ed995447

[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o Unified_cpp_dom_base8.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/extensions/permissions -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/netwerk/url-classifier -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/atk-1.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base8.o.pp Unified_cpp_dom_base8.cpp
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - In file included from Unified_cpp_dom_base8.cpp:38:
[task 2020-02-21T20:56:36.113Z] 20:56:36 ERROR - /builds/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:2834:10: error: no member named 'mBrowsingContext' in 'nsFrameLoader'
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - aDest->mBrowsingContext->EnsureAttached();
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - ~~~~~ ^
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - 1 error generated.
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - /builds/worker/workspace/build/src/config/rules.mk:731: recipe for target 'Unified_cpp_dom_base8.o' failed
[task 2020-02-21T20:56:36.113Z] 20:56:36 ERROR - make[4]: *** [Unified_cpp_dom_base8.o] Error 1
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - make[4]: *** Waiting for unfinished jobs....
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/xul'
[task 2020-02-21T20:56:36.113Z] 20:56:36 INFO - dom/xul/Unified_cpp_dom_xul1.o

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0cc45b5d6e5
Part 1: Return a raw pointer from Get[Extant]BrowsingContext, r=kmag
https://hg.mozilla.org/integration/autoland/rev/404c016b3eaa
Part 2: Call GetBrowsingContext from ApplySandboxFlags, r=kmag
https://hg.mozilla.org/integration/autoland/rev/2b4b772b60e7
Part 3: Rename nsFrameLoader::mBrowsingContext to avoid confusion, r=kmag

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/f0cc45b5d6e5
https://hg.mozilla.org/mozilla-central/rev/404c016b3eaa
https://hg.mozilla.org/mozilla-central/rev/2b4b772b60e7

Comment 12

[Julien Cristau [:jcristau]]

Looks like this crash is gone in 74.0b6+. Should we mark beta as fixed by the backout of bug 1582832?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

That seems reasonable to me.