Closed Bug 1367686 Opened 4 years ago Closed 3 years ago

Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild

Categories

(Core :: Widget: Win32, defect)

53 Branch
All
Windows
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 1373220
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: philipp, Unassigned)

References

Details

(5 keywords, Whiteboard: [adv-main56-])

Crash Data

This bug was filed from the Socorro interface and is 
report bp-2c96ec93-7fd2-467f-8ea0-cce6b0170428.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/glue/nsCOMPtr.cpp:44
1 	xul.dll 	nsBaseWidget::AddChild(nsIWidget*) 	widget/nsBaseWidget.cpp:627
2 	xul.dll 	nsWindow::SetParent(nsIWidget*) 	widget/windows/nsWindow.cpp:1237
3 	xul.dll 	nsPluginFrame::EndSwapDocShells(nsISupports*, void*) 	layout/generic/nsPluginFrame.cpp:1860
4 	xul.dll 	nsIDocument::EnumerateActivityObservers(void (*)(nsISupports*, void*), void*) 	dom/base/nsDocument.cpp:9797
5 	xul.dll 	EndSwapDocShellsForDocument 	layout/generic/nsSubDocumentFrame.cpp:1149
6 	xul.dll 	EndSwapDocShellsForViews 	layout/generic/nsSubDocumentFrame.cpp:1161
7 	xul.dll 	nsSubDocumentFrame::EndSwapDocShells(nsIFrame*) 	layout/generic/nsSubDocumentFrame.cpp:1192
8 	xul.dll 	nsFrameLoader::SwapWithOtherLoader(nsFrameLoader*, nsIFrameLoaderOwner*, nsIFrameLoaderOwner*) 	dom/base/nsFrameLoader.cpp:1898
9 	xul.dll 	nsXULElement::SwapFrameLoaders(nsIFrameLoaderOwner*, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1735
10 	xul.dll 	nsXULElement::SwapFrameLoaders(nsXULElement&, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1715
11 	xul.dll 	mozilla::dom::XULElementBinding::swapFrameLoaders 	obj-firefox/dom/bindings/XULElementBinding.cpp:8228
12 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:460
13 	xul.dll 	InternalCall 	js/src/vm/Interpreter.cpp:505
14 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:2989
15 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:406
16 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:478
17 	xul.dll 	js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp:524
18 	xul.dll 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp:2847
19 	xul.dll 	JS::StructGCPolicy<JS::GCVector<jsid, 8, js::TempAllocPolicy> >::trace(JSTracer*, JS::GCVector<jsid, 8, js::TempAllocPolicy>*, char const*) 	obj-firefox/dist/include/js/GCPolicyAPI.h:87
20 	xul.dll 	js::TraceManuallyBarrieredEdge<JSObject*>(JSTracer*, JSObject**, char const*) 	js/src/gc/Marking.cpp:463
21 	xul.dll 	js::TraceManuallyBarrieredGenericPointerEdge(JSTracer*, js::gc::Cell**, char const*) 	js/src/gc/Marking.cpp:653
22 	xul.dll 	mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) 	obj-firefox/dom/bindings/EventHandlerBinding.cpp:259

this crash signature on windows is regressing in volume with firefox 53 - it's currently accounting for 0.25% of browser crashes on 53.0.3.

many user comments talk about handling tabs with playing media content at the time of the crash:
* bp-5d1a05e1-6fca-4b75-96d4-dfc3f0170505: drag tab from non-maximised window to maximised window -> crash
* bp-86afc3f0-b478-455f-89a7-6652b0170428: While moving a tab from one window to another, both windoiws disappeared
* bp-5cee96d5-3061-40f6-ba04-222090170501: had two windows open, one with a flash game the other with a video player on hulu. both on seperate monitors. when I took the video player out of fullscreen the browser crashed
* bp-856eac3b-b835-4602-9805-ae5a00170511: Was loading up various Twitch streams on a multi monitor setup. Small chance it is hardware related due to having a workstation as a desktop.
* bp-367c89b3-cbfb-4785-9ce4-3596b0170426: Dragging/combining a tab with running video, into another Firefox window.
* bp-72be76ed-0985-4edd-837d-353170170512: Crashed when moving tab between monitors.
* bp-6c41f42b-dd99-4ac3-97bb-28dfd0170522: Fourth or fifth time this has happened. All I'm doing is moving a tab that has a stream open, from one monitor (where it was by itself) to another (joined with several other tabs).
Component: Untriaged → XPCOM
UAF in widget code it appears.  High frequency, URLs all over the place, but it's possible video is a common factor (perhaps in a non-foreground tab)

Apparently no hits before 53
Group: core-security
Component: XPCOM → Widget: Win32
Similar fails in RemoveChild
Crash Signature: [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild] → [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild] [@ nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::RemoveChild]
Jimm, Boris - any thoughts?  Suggestions who should look at this?
Flags: needinfo?(jmathies)
Flags: needinfo?(bzbarsky)
So we crash at 0x14 when calling mLastChild->SetNextSibling(aChild).  That should be calling into nsIWidget::SetNextSibling which just does "mNextSibling = aSibling".

So presumably mLastChild is null.  But we null-checked mFirstChild and it was not null...

The only places we modify mFirstChild are nsBaseWidget::AddChild, nsBaseWidget::RemoveChild, and nsBaseWidget::SetZIndex.  AddChild sets mLastChild to the same thing as mFirstChild.  SetZIndex sets mFirstChild to non-null.

RemoveChild does look a bit fishy in that if the caller doesn't hold a ref to aChild it could die when we set mFirstChild to null and then who knows what might happen...

Do we have anything like a regression range?
Flags: needinfo?(bzbarsky)
I checked, and almost all the crashes from comment 0 are at 0x14.  The one exception is https://crash-stats.mozilla.com/report/index/856eac3b-b835-4602-9805-ae5a00170511 which is at 0x34353535
Group: core-security → layout-core-security
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #5)
> I checked, and almost all the crashes from comment 0 are at 0x14.  The one
> exception is
> https://crash-stats.mozilla.com/report/index/856eac3b-b835-4602-9805-
> ae5a00170511 which is at 0x34353535

aka "3555" -- looks like a string -- implies null is not guaranteed, and it might be modifiable to point somewhere interesting.  Or at least not a clean null-deref
Duplicate of this bug: 1359390
See Also: → 1373220
Noting this is the #3 topcrash for 55 beta.   452 crashes in the last week so the volume of this signature is higher on beta than on release.
Note we're definitely hitting true UAFs here.  Clearly looks like the widget has been released.  Windows-only it appears (not shocking in widget code).
For nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild on Beta:
(98.49% in signature vs 01.47% overall) Addon "Firebug" = true
The comments frequently mention closing the Firebug menu (maybe from the windows taskbar with Firefox out of focus). This is the most worrisome crash in 55 beta at this point. 
Kamil, I wonder if you can reproduce the crash by fiddling around with Firebug and windows?
Flags: needinfo?(kjozwiak)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(jmathies)
Resolution: --- → DUPLICATE
Duplicate of bug: 1373220
Flags: needinfo?(kjozwiak)
Mark 56 fixed as bug 1373220 was fixed in 56.
Whiteboard: [adv-main56-]
Group: layout-core-security
You need to log in before you can comment on or make changes to this bug.