Closed Bug 1595936 Opened 5 years ago Closed 5 years ago

MOZ_CRASH(This is unsafe! Fix the caller!) at dom/events/EventDispatcher.cpp:805

Categories

(Core :: DOM: Content Processes, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla74
Fission Milestone M5
Tracking Status
firefox74 --- fixed

People

(Reporter: hiro, Assigned: kmag)

References

Details

Attachments

(6 files)

Whenever I run devtools/client/responsive/test/browser/browser_viewport_resizing_after_reload.js, I get this crash;

a back trace

#0 0x00007fffe81f8b0f in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>) (aTarget=<optimized out>, aPresContext=0x0, aEvent=0x7fffcf4411f0, aDOMEvent=0x7fffc6703e80, aEventStatus=0x7fffffff5904, aCallback=0x0, aTargets=0x0) at /home/hiro/central/dom/events/EventDispatcher.cpp:805
#1 0x00007fffe81f9b18 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports
, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) (aTarget=0x7fffde48b8a0, aEvent=<optimized out>, aDOMEvent=0x7fffc6703e80, aPresContext=0x0, aEventStatus=<optimized out>)
at /home/hiro/central/dom/events/EventDispatcher.cpp:1146
#2 0x00007fffe81e30f6 in mozilla::DOMEventTargetHelper::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) (this=0x7ffff706e680 <_IO_2_1_stderr_>, aEvent=..., aCallerType=mozilla::dom::CallerType::NonSystem, aRv=...) at /home/hiro/central/dom/events/DOMEventTargetHelper.cpp:169
#3 0x00007fffe820104f in mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) (this=0x7fffde48b8a0, aEvent=...) at /home/hiro/central/dom/events/EventTarget.cpp:178
#4 0x00007fffe89e9ef8 in mozilla::dom::ipc::SharedMap::Update(mozilla::ipc::FileDescriptor const&, unsigned long, nsTArray<RefPtr<mozilla::dom::BlobImpl> >&&, nsTArray<nsTString<char> >&&) (this=<optimized out>, aMapFile=..., aMapSize=<optimized out>, aBlobs=..., aChangedKeys=...) at /home/hiro/central/dom/ipc/SharedMap.cpp:136
#5 0x00007fffe89eb05b in mozilla::dom::ipc::WritableSharedMap::BroadcastChanges() (this=0x7fffdb14f160)
at /home/hiro/central/dom/ipc/SharedMap.cpp:373
#6 0x00007fffe89be41c in mozilla::dom::ContentParent::InitInternal(mozilla::hal::ProcessPriority) (this=<optimized out>, aInitialPriority=mozilla::hal::PROCESS_PRIORITY_FOREGROUND)
at /home/hiro/central/dom/ipc/ContentParent.cpp:2533
#7 0x00007fffe89bd277 in mozilla::dom::ContentParent::LaunchSubprocessInternal(mozilla::hal::ProcessPriority, mozilla::Variant<bool*, RefPtr<mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>, mozilla::ipc::LaunchError, false> >>&&)::$_16::operator()(int) const (this=0x7fffffff6238, handle=15071)
at /home/hiro/central/dom/ipc/ContentParent.cpp:2230
#8 0x00007fffe89bcef7 in mozilla::dom::ContentParent::LaunchSubprocessInternal(mozilla::hal::ProcessPriority, mozilla::Variant<bool
, RefPtr<mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>, mozilla::ipc::LaunchError, false> >>&&) (this=0x7fffcf443000, aInitialPriority=mozilla::hal::PROCESS_PRIORITY_FOREGROUND, aRetval=...)
at /home/hiro/central/dom/ipc/ContentParent.cpp:2275
#9 0x00007fffe89b92e4 in mozilla::dom::ContentParent::LaunchSubprocessSync(mozilla::hal::ProcessPriority) (this=0x7ffff706e680 <_IO_2_1_stderr_>, aInitialPriority=-150538064)
at /home/hiro/central/dom/ipc/ContentParent.cpp:2298
#10 0x00007fffe8997326 in mozilla::dom::ContentParent::GetNewOrUsedBrowserProcess(mozilla::dom::Element
, nsTSubstring<char16_t> const&, mozilla::hal::ProcessPriority, mozilla::dom::ContentParent*, bool) (aFrameElement=<optimized out>, aRemoteType=..., aPriority=<optimized out>, aOpener=0x0, aPreferUsed=false)
at /home/hiro/central/dom/ipc/ContentParent.cpp:928
#11 0x00007fffe89b9b8f in mozilla::dom::ContentParent::CreateBrowser(mozilla::dom::TabContext const&, mozilla::dom::Element*, nsTSubstring<char16_t> const&, mozilla::dom::BrowsingContext*, mozilla::dom::ContentParent*, mozilla::dom::BrowserParent*, unsigned long) (aContext=..., aFrameElement=<optimized out>, aRemoteType=..., aBrowsingContext=0x7fffcf7690e0, aOpenerContentParent=<optimized out>, aSameTabGroupAs=
0x0, aNextRemoteTabId=<optimized out>) at /home/hiro/central/dom/ipc/ContentParent.cpp:1172
#12 0x00007fffe732fe81 in nsFrameLoader::TryRemoteBrowserInternal() (this=0x7fffc66e7820)
at /home/hiro/central/dom/base/nsFrameLoader.cpp:2644
#13 0x00007fffe732f739 in nsFrameLoader::TryRemoteBrowser() (this=0x7fffc66e7820)
at /home/hiro/central/dom/base/nsFrameLoader.cpp:2708
#14 0x00007fffe732f170 in nsFrameLoader::GetBrowsingContext() (this=0x7fffc66e7820)
at /home/hiro/central/dom/base/nsFrameLoader.cpp:3246
#15 0x00007fffe82dea7f in mozilla::dom::HTMLIFrameElement::MaybeStoreCrossOriginFeaturePolicy() (this=0x7fffc66e7580) at /home/hiro/central/dom/html/HTMLIFrameElement.cpp:243
#16 0x00007fffe82de48d in mozilla::dom::HTMLIFrameElement::RefreshFeaturePolicy(bool) (this=0x7fffc66e7580, aParseAllowAttribute=<optimized out>) at /home/hiro/central/dom/html/HTMLIFrameElement.cpp:321
#17 0x00007fffe82de1f8 in mozilla::dom::HTMLIFrameElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc66e7580, aContext=..., aParent=...) at /home/hiro/central/dom/html/HTMLIFrameElement.cpp:77
#18 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716820, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#19 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716820, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
#20 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716b80, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#21 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716b80, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
---Type <return> to continue, or q <return> to quit---
#22 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716c10, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#23 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716c10, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
#24 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716d30, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#25 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716d30, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
#26 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716e50, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#27 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716e50, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
#28 0x00007fffe7241b68 in mozilla::dom::Element::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716f70, aContext=..., aParent=...) at /home/hiro/central/dom/base/Element.cpp:1555
#29 0x00007fffe836d4e3 in nsGenericHTMLElement::BindToTree(mozilla::dom::BindContext&, nsINode&) (this=0x7fffc6716f70, aContext=..., aParent=...) at /home/hiro/central/dom/html/nsGenericHTMLElement.cpp:376
#30 0x00007fffe733b1b5 in nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool) (this=0x7fffcf5589d0, aKid=0x7fffc6716f70, aChildToInsertBefore=<optimized out>, aNotify=<optimized out>)
at /home/hiro/central/dom/base/nsINode.cpp:1361
#31 0x00007fffe733e084 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) (this=<optimized out>, aReplace=<optimized out>, aNewChild=0x7fffc6716f70, aRefChild=<optimized out>, aError=...)
at /home/hiro/central/dom/base/nsINode.cpp:2475
#32 0x00007fffe7630e5b in nsINode::InsertBefore(nsINode&, nsINode*, mozilla::ErrorResult&) (this=0x7fffcf5589d0, aNode=..., aChild=0x0, aError=...) at /home/hiro/central/dom/base/nsINode.h:1832
#33 0x00007fffe7630e5b in nsINode::AppendChild(nsINode&, mozilla::ErrorResult&) (this=0x7fffcf5589d0, aNode=..., aError=...) at /home/hiro/central/dom/base/nsINode.h:1835
#34 0x00007fffe7630e5b in mozilla::dom::Node_Binding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) (cx=<optimized out>, obj=..., self=0x7fffcf5589d0, args=...) at NodeBinding.cpp:953
#35 0x00007fffe7f81972 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff706e680 <_IO_2_1_stderr_>, argc=<optimized out>, vp=<optimized out>)
at /home/hiro/central/dom/bindings/BindingUtils.cpp:3218
#36 0x00007fffea21c8c2 in CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) (cx=0x7fffdca31000, native=0x7fffe7f817eb <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...) at /home/hiro/central/js/src/vm/Interpreter.cpp:456
#37 0x00007fffea20b918 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=0x7fffdca31000, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
at /home/hiro/central/js/src/vm/Interpreter.cpp:548
#38 0x00007fffea200795 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7fffdca31000, args=...)
at /home/hiro/central/js/src/vm/Interpreter.cpp:621
#39 0x00007fffea200795 in Interpret(JSContext*, js::RunState&) (cx=0x7fffdca31000, state=...)
at /home/hiro/central/js/src/vm/Interpreter.cpp:3110
#40 0x00007fffea1f68d1 in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffdca31000, state=...)
at /home/hiro/central/js/src/vm/Interpreter.cpp:423
#41 0x00007fffea20b93b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=0x7fffdca31000, args=..., construct=<optimized out>, reason=(unknown: -822809600))
at /home/hiro/central/js/src/vm/Interpreter.cpp:589
#42 0x00007fffeac7e765 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (cx=0x7fffdca31000, frame=0x7fffffff8308, stub=<optimized out>, argc=1, vp=0x7fffffff8290, res=...) at /home/hiro/central/js/src/jit/BaselineIC.cpp:2940

and js call stack;

0 appendChildToContainer(container = "[object HTMLDivElement]", child = "[object HTMLDivElement]") ["resource://devtools/client/shared/vendor/react-dom.js":6493:15]
1 commitPlacement(finishedWork = "[object Object]") ["resource://devtools/client/shared/vendor/react-dom.js":13374:32]
2 commitAllHostEffects() ["resource://devtools/client/shared/vendor/react-dom.js":14112:25]
3 commitRoot(root = "[object Object]", finishedWork = "[object Object]") ["resource://devtools/client/shared/vendor/react-dom.js":14344:8]
4 completeRoot/<() ["resource://devtools/client/shared/vendor/react-dom.js":15731:14]
5 unstable_runWithPriority(priorityLevel = "1", eventHandler = [function]) ["resource://devtools/client/shared/vendor/react.js":617:11]
6 completeRoot(root = "[object Object]", finishedWork = "[object Object]", expirationTime = "1073741823") ["resource://devtools/client/shared/vendor/react-dom.js":15730:26]
7 performWorkOnRoot(root = "[object Object]", expirationTime = "1073741823", isYieldy = "false") ["resource://devtools/client/shared/vendor/react-dom.js":15659:20]
8 performWork(minExpirationTime = "1073741823", isYieldy = "false") ["resource://devtools/client/shared/vendor/react-dom.js":15567:23]
9 performSyncWork() ["resource://devtools/client/shared/vendor/react-dom.js":15541:13]
10 requestWork(root = "[object Object]", expirationTime = "1073741823") ["resource://devtools/client/shared/vendor/react-dom.js":15410:4]
11 scheduleWork(fiber = "[object Object]", expirationTime = "1073741823") ["resource://devtools/client/shared/vendor/react-dom.js":15224:15]
12 enqueueSetState(inst = "[object Object]", payload = "[object Object]", callback = "undefined", ""setState"") ["resource://devtools/client/shared/vendor/react-dom.js":8192:16]
this = [object Object]
13 Component.prototype.setState(partialState = "[object Object]") ["resource://devtools/client/shared/vendor/react.js":328:15]
this = [object Object]
14 onStateChange() ["resource://devtools/client/shared/vendor/react-redux.js":1412:15]
this = [object Object][Thread 0x7fffce7eb700 (LWP 14933) exited]

As the assertion says, fix the caller.

Component: DOM: Events → DOM: Content Processes

kmag, I don't know where/how SharedMap's change event is used, but could we possibly dispatch it using AsyncEventDispatcher's RunDOMEventWhenSafe()?
If so, happy to write a patch (or review a patch).

Other option is to Flush() when safe https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/dom/ipc/ContentParent.cpp#2537
And harder one would be to make all remote browser creation to happen when it is safe to run scripts.

Flags: needinfo?(kmaglione+bmo)

(In reply to Olli Pettay [:smaug] from comment #2)

kmag, I don't know where/how SharedMap's change event is used, but could we possibly dispatch it using AsyncEventDispatcher's RunDOMEventWhenSafe()?
If so, happy to write a patch (or review a patch).

Other option is to Flush() when safe https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/dom/ipc/ContentParent.cpp#2537
And harder one would be to make all remote browser creation to happen when it is safe to run scripts.

We can dispatch the event using AsyncEventDispatcher, yes. We can't delay calling Flush, since the SharedMap API is meant to guarantee that any new processes start with the most current shared data state, and delaying the flush would potentially break that guarantee.

I am a bit worried that we don't already guarantee that FrameLoader creation doesn't happen during a document update in this case, though, given how much trouble we go to normally to only do it from MaybeInitializeFinalizeFrameLoaders() at the end of an update...

Flags: needinfo?(kmaglione+bmo)

I think we really probably want to move the RefreshFeaturePolicy call from HTMLIFrameElement::BindToTree to BrowsingContext::SetEmbedderElement so we don't force FrameLoader creation to happen earlier than intended.

Indeed.

I guess all the callers of nsFrameLoader::GetBrowsingContext() need to be audited, and that method renamed.

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jmathies)

I think there are a few things we probably want to change here:

  1. Make this sanity check understand chrome EventTargets which aren't part of a document. It's only supposed to crash for listeners which may run content JS. Chrome targets are supposed to only issue a warning. But its check for chrome targets doesn't work for listeners that live in the JSM scope, so it takes the stricter path.

  2. Add a diagnostic assert that we don't try to create a FrameLoader when it isn't safe to run scripts, since we generally try pretty hard to avoid that. That would have caught this issue much sooner, rather than when we just happened to try to fire an event during FrameLoader creation.

  3. Fix the RefreshFeaturePolicy call in BindToTree.

I suppose the first two are probably bugs for me, so I guess I'll take the third one as well.

Assignee: nobody → kmaglione+bmo
Flags: needinfo?(jmathies)
Priority: -- → P3
Status: NEW → ASSIGNED
Fission Milestone: --- → M5
Priority: P3 → P2

We go to great lengths to try to avoid initializing FrameLoaders during
document updates. That means that when BindToTree is called, the element's
FrameLoader is not initialized, and it has no BrowsingContext. Calling
GetBrowsingContext() (which happens as a side-effect of
HTMLIFrameElement::RefreshFeaturePolicy), however, forces eager
initialization, which can cause any number of problems.

This patch moves that logic from being triggered by BindToTree to being
triggered by BrowsingContext::Embed, which happens as soon as the
BrowsingContext is bound to the element, but does not force it to be created
early.

We go to some lengths to defer FrameLoader initialization to the end of
document updates. However, it is still possible for initialization to happen
earlier as a side-effect of other operations (such as
nsFrameLoader::GetBrowsingContext()) if they're called before initialization
has already happened. Since this is such an easy mistake to make, we should
assert that it doesn't happen so that we find out about it sooner rather than
later.

Pushed by maglione.k@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7dfb10990e23 Part 1 - Warn for unsafe event dispatch in non-document system globals. r=smaug https://hg.mozilla.org/integration/autoland/rev/39ab7a16e4fa Part 2 - Refresh feature policy when attaching browsing context. r=farre,baku https://hg.mozilla.org/integration/autoland/rev/d28972865969 Part 3a - Don't force early FrameLoader initialization in recursion check. r=smaug https://hg.mozilla.org/integration/autoland/rev/9d1ee006a479 Part 3b - Don't force FrameLoader initialization on removal. r=smaug https://hg.mozilla.org/integration/autoland/rev/26fa20256e6d Part 3c - Dispatch message manager observers only when safe. r=smaug https://hg.mozilla.org/integration/autoland/rev/8e70567acc3b Part 4 - Assert that we don't init FrameLoaders when unsafe. r=smaug

I'm so pleased to see this, thank you!

Regressions: 1644365
Regressions: 1662259
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: