Closed Bug 1612431 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 + fixed

People

(Reporter: tsmith, Assigned: smaug)

References

(Blocks 1 open bug, Regression)

Details

(5 keywords, Whiteboard: [fuzzblocker][post-critsmash-triage][adv-main74+r])

Crash Data

Attachments

(2 files)

Attached file testcase.html (deleted) —

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

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

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

Component: DOM: Core & HTML → DOM: Navigation
Assignee: nobody → bugs
Priority: -- → P1

Carrying over the tracking flag from bug 1611764.

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

Crash Signature: [@ nsFrameLoader::MaybeCreateDocShell ]

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

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

Keywords: sec-high

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
Attachment #9124387 - Flags: sec-approval?
Keywords: sec-highsec-moderate
Regressed by: 1578628
Has Regression Range: --- → yes
Keywords: regression

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

Attachment #9124387 - Flags: sec-approval? → sec-approval+
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: qe-verify-
Whiteboard: [fuzzblocker] → [fuzzblocker][post-critsmash-triage]
Whiteboard: [fuzzblocker][post-critsmash-triage] → [fuzzblocker][post-critsmash-triage][adv-main74+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: