722155 - crash nsBaseWidget::SetZIndex

Status: RESOLVED WORKSFORME

(Core :: Widget: Win32, defect, P5)

Description

[Mats Palmgren (inactive)]

bp-2603317e-722e-41ea-9978-8ef592120127

crash only occurs on Windows; all versions back to 3.5

nsBaseWidget::SetZIndex	
UpdateNativeWidgetZIndexes	view/src/nsView.cpp:650
nsView::InitializeWindow	view/src/nsView.cpp:847
nsView::CreateWidgetForPopup	view/src/nsView.cpp:828
nsMenuPopupFrame::CreateWidgetForView	layout/xul/base/src/nsMenuPopupFrame.cpp:333
nsMenuPopupFrame::Init	
nsCSSFrameConstructor::InitAndRestoreFrame	layout/base/nsCSSFrameConstructor.cpp:4524
nsCSSFrameConstructor::ConstructFrameFromItemInternal	layout/base/nsCSSFrameConstructor.cpp:3741
nsCSSFrameConstructor::ConstructFramesFromItem	layout/base/nsCSSFrameConstructor.cpp:5505
nsCSSFrameConstructor::ConstructFramesFromItemList	layout/base/nsCSSFrameConstructor.cpp:9528
nsCSSFrameConstructor::ContentRangeInserted	layout/base/nsCSSFrameConstructor.cpp:7194
nsCSSFrameConstructor::RecreateFramesForContent	layout/base/nsCSSFrameConstructor.cpp:9167
nsCSSFrameConstructor::MaybeRecreateFramesForElement	layout/base/nsCSSFrameConstructor.cpp:8929
mozilla::css::RestyleTracker::DoProcessRestyles	layout/base/RestyleTracker.cpp:242
PresShell::FlushPendingNotifications	layout/base/nsPresShell.cpp:4073
nsBoxObject::GetFrame	layout/xul/base/src/nsBoxObject.cpp:158
nsPopupBoxObject::GetPopupState	layout/xul/base/src/nsPopupBoxObject.cpp:241
NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
XPCWrappedNative::GetAttribute	js/xpconnect/src/xpcprivate.h:2656
XPC_WN_GetterSetter	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1587
js::InvokeKernel	js/src/jsinterp.cpp:520
js::Invoke	js/src/jsinterp.cpp:570
js::InvokeGetterOrSetter	js/src/jsinterp.cpp:644
js::Shape::get	js/src/jsscopeinlines.h:287
js::GetPropertyHelper	js/src/jsobj.cpp:5384 
...

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

I think this problem is that mWidget isn't initialized to NULL in the constructor for nsView.

The following code causes the crash:

>  if (aView->HasWidget()) {
>    nsIWidget* widget = aView->GetWidget();
>    PRInt32 curZ;
>    widget->GetZIndex(&curZ);
>    if (curZ != aZIndex) {
>      widget->SetZIndex(aZIndex); <------------------------
>    }

The crash would sometimes happen when aView's mWidget value happens to be initialized to a non-NULL value.  SetZIndex does a lot more work than GetZIndex including using the 'this' pointer.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 593308 [details] [diff] [review]
Patch v1.

Review of attachment 593308 [details] [diff] [review]:
-----------------------------------------------------------------

huh.

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks for the super quick review :)

Comment 4

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/253a5461dec1

Comment 5

[Mats Palmgren (inactive)]

class nsView has NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW so I think those
null-assignments were intentionally omitted.  In fact, we should remove
the other ones too and leave a comment that such initializations are
redundant.  Anyway, the patch wont fix the crash.

Please leave the bug open when merging to mozilla-central (if that occurs).

Comment 6

[Brian R. Bondy [:bbondy]]

Backed out
http://hg.mozilla.org/integration/mozilla-inbound/rev/f09662d2ba1f

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I hate NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW for exactly this reason.

Comment 8

[Brian R. Bondy [:bbondy]]

Sorry about that. I knew about operator new, but not the macro. So I didn't notice it :(

Comment 9

[Brian R. Bondy [:bbondy]]

Also NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW is s a bit dangerous in general, but not in this case since other classes could use it who don't get heap allocated hence not properly initialized.

Comment 10

[Timothy Nikkel (:tnikkel)]

This seems to fall into a class of crashes that I've seen where we crash shortly after creating a new widget and the only obvious cause is that the widget didn't get created properly even though we check error codes and return values for proper creation everywhere that is appropriate (that I've seen so far).

Comment 11

[Mats Palmgren (inactive)]

QA: does crash-stats give any clues in comments, URLs or add-ons for reproducing this?
(including the signature in bug 837499)

Comment 12

[Robert Kaiser]

URLs from https://crash-stats.mozilla.com/report/list?signature=nsBaseWidget%3A%3ASetZIndex%28int%29 are as follows:
443 	about:blank
87 	about:home
18 	https://www.facebook.com/
5 	http://www.facebook.com/
4 	https://es-es.facebook.com/
(there's more with a single hit only, but nothing special there either)

Correlations also don't bring up anything interesting, unfortunately.

Comment 13

[Bogdan Maris, Desktop QA]

Firefox 21.0 RC (buildID: 20130511120803)
Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20100101 Firefox/21.0

Firefox 22.0 RC (buildID: 20130618035212)
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0

Latest Nightly (buildID: 20130723030205)
Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20130723 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20130723 Firefox/25.0

Firefox 18.0.1 (buildID: 20130116073211)
Mozilla/5.0 (Windows NT 5.1; rv:18.0) Gecko/20100101 Firefox/18.0
Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/20100101 Firefox/18.0

Tested on the above builds using the links from comment 12 but I was unable to reproduce the crash. I tested with BrowsetoSave plugin, Gmail and whatever I could find in the comments from crash-reports. I noticed that there are 101 crashes on Windows XP, 60 crashes on Windows 7 and 22 crashes on Windows Vista with the same signature in the last 7 days. Is there something else I can help with here?

Comment 14

[Wayne Mery (:wsmwk)]

97% of current crashes are startup.

bp-e4c47600-44e6-4b90-a4e4-41b612140825 version 32 example
 0 	xul.dll	nsBaseWidget::SetZIndex(int)	widget/xpwidgets/nsBaseWidget.cpp
1 	xul.dll	UpdateNativeWidgetZIndexes	view/src/nsView.cpp
2 	xul.dll	nsView::InitializeWindow(bool, bool)	view/src/nsView.cpp
3 	xul.dll	nsView::CreateWidgetForPopup(nsWidgetInitData*, nsIWidget*, bool, bool)	view/src/nsView.cpp
4 	xul.dll	nsMenuPopupFrame::CreateWidgetForView(nsView*)	layout/xul/nsMenuPopupFrame.cpp

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsBaseWidget::SetZIndex':
 - nightly (version 51): 0 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 36 crashes from 2016-08-02.
 - release (version 48): 48 crashes from 2016-07-25.
 - esr     (version 45): 5 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       0       0       0
 - aurora        0       0       0
 - beta         10      13       5
 - release      16      17       4
 - esr           1       0       1

Affected platform: Windows

Crash rank on the last 7 days:
           Browser     Content   Plugin
 - nightly
 - aurora
 - beta    #1282
 - release #1117
 - esr

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.