1612431 - Assertion failure: nsContentUtils::IsSafeToRunScript() || mOwnerContent->OwnerDoc()->IsStaticDocument() (FrameLoader should never be initialized during document update or reflow!), at src/dom/base/nsFrameLoader.cpp:1977

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c 20200130-f97c48da9cee

This was first seen with m-c 20200125-f7f534f08b48 and is happening frequently.

Assertion failure: nsContentUtils::IsSafeToRunScript() || mOwnerContent->OwnerDoc()->IsStaticDocument() (FrameLoader should never be initialized during document update or reflow!), at src/dom/base/nsFrameLoader.cpp:1977

#0 nsFrameLoader::AssertSafeToInit() src/dom/base/nsFrameLoader.cpp:1974:3
#1 nsFrameLoader::MaybeCreateDocShell() src/dom/base/nsFrameLoader.cpp:1989:3
#2 nsFrameLoader::GetBrowsingContext() src/dom/base/nsFrameLoader.cpp:3215:15
#3 nsGenericHTMLFrameElement::AfterMaybeChangeAttr(int, nsAtom*, nsAttrValueOrString const*, nsIPrincipal*, bool) src/dom/html/nsGenericHTMLFrameElement.cpp:344:40
#4 nsGenericHTMLFrameElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/html/nsGenericHTMLFrameElement.cpp:285:5
#5 mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, mozilla::dom::Document*, mozAutoDocUpdate const&) src/dom/base/Element.cpp:2339:10
#6 mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/dom/base/Element.cpp:2199:10
#7 SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:830:12
#8 SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:826:12
#9 SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:1564:14
#10 SetHTMLAttr src/dom/html/nsGenericHTMLElement.h:718:5
#11 mozilla::dom::HTMLIFrameElement::SetName(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) src/obj-firefox/dist/include/mozilla/dom/HTMLIFrameElement.h:67:5
#12 mozilla::dom::HTMLIFrameElement_Binding::set_name(JSContext*, JS::Handle<JSObject*>, void*, JSJitSetterCallArgs) src/obj-firefox/dom/bindings/HTMLIFrameElementBinding.cpp:266:24
#13 bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3099:8
#14 CallJSNative src/js/src/vm/Interpreter.cpp:450:13
#15 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:542:12
#16 InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:605:10
#17 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:622:8
#18 js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/vm/Interpreter.cpp:760:10
#19 SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) src/js/src/vm/NativeObject.cpp:2956:8
#20 bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) src/js/src/vm/NativeObject.cpp:2985:14
#21 js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) src/js/src/vm/ObjectOperations-inl.h:283:10
#22 js::jit::DoSetPropFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICSetProp_Fallback*, JS::Value*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/jit/BaselineIC.cpp:2704:10

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/Q3p6zrNZoFMl5aTnhLGJPA/index.html

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I think this is the same as one existing bug, which I can't now find.
The behavior of nsFrameLoader::GetBrowsingContext() is basically nuts. It shouldn't create anything. Right now it is very error prone to use.
This is kind of a regression from bug 1578628.
The old code was explicitly using GetExistingDocShell()

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Carrying over the tracking flag from bug 1611764.

Comment 5

[Pascal Chevrel:pascalc (PTO until April 26)]

Carrying over the crash signature from bug 1611764 so we don't loose the stability impact.

Comment 6

[Bob Clary [:bc] (inactive)]

Bughunter found this on https://www.optumbank.com/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1612431, it is enough to update existing browsing context name when changing iframe.name, r=kmag

This brings back some of the behavior we had before bug 1578628.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9124387 [details]
Bug 1612431, it is enough to update existing browsing context name when changing iframe.name, r=kmag

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The crash is just an assertion, but that does hint about unexpected behavior.
  Because of the problematic behavior we may end up running chrome during a time when it is not expected. I haven't found a way to run content JS.
  Perhaps this is just sec-mod after all?
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 71 and higher
• If not all supported branches, which bug introduced the flaw?: Bug 1578628
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: the patch should work with beta given how trivial it is.
• How likely is this patch to cause regressions; how much testing does it need?: The patch should bring back the old behavior

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9124387 [details]
Bug 1612431, it is enough to update existing browsing context name when changing iframe.name, r=kmag

sec-approval+ dveditz

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/656b57e594def0996f3f5518f36c6919636d9209
https://hg.mozilla.org/mozilla-central/rev/656b57e594de