Closed Bug 1615480 Opened 2 months ago Closed 1 month ago

Crash due to SetSandboxFlags on BrowsingContext when !EverAttached()

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla75
Fission Milestone M5
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- wontfix
firefox74 + fixed
firefox75 + fixed

People

(Reporter: mccr8, Assigned: Nika)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

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

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

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.

No longer depends on: 1615403

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

Keywords: regression
Regressed by: 1582832

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

Regressed by: 1587062
No longer regressed by: 1582832
Summary: Crash in [@ mozilla::dom::PContentChild::SendCommitBrowsingContextTransaction] → Crash due to SetSandboxFlags on BrowsingContext when !EverAttached()
Assignee: nobody → nika

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.

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

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.

Fission Milestone: --- → M5
Priority: -- → P1
Pushed by nlayzell@mozilla.com:
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

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

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com:
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
Flags: needinfo?(nika)

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

Flags: needinfo?(nika)

That seems reasonable to me.

Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.