503391 - [Mac 10.4] Crash in [@ nsViewManager::IsViewInserted(nsView*)] using Google Toolbar

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Marcia Knous [:marcia]]

Seen while running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 GTB5

STR:
1. Install the latest version of Google Toolbar. Retain all of the default settings.
2. Type at least 7 letters in the Google search box. Sometimes I have to backspace a few times to initiate the crash.

http://tinyurl.com/l7pta5 has the list of crashes to date for 3.5. It currently ranks as the #9 top crash on Mac. This is 10.4 only and does not affect 10.5. One of my sample breakpad reports: http://crash-stats.mozilla.com/report/index/1c0b98a4-3c03-42ab-bcef-cf5492090709

mw22 indicates he will work on a reduced test case later this afternoon.

Comment 1

[Marcia Knous [:marcia]]

Attachment: testcase

This crashes after 1 second.

I get this stacktrace constantly with it:
http://crash-stats.mozilla.com/report/index/03130994-0d9e-4d80-8f58-7dafa2090709
0  	 	@0x0  	
1 	XUL 	nsXULPopupManager::HidePopupCallback 	layout/xul/base/src/nsXULPopupManager.cpp:761
2 	XUL 	nsXULPopupManager::FirePopupHidingEvent 	layout/xul/base/src/nsXULPopupManager.cpp:1096
3 	XUL 	nsXULPopupManager::HidePopup 	layout/xul/base/src/nsXULPopupManager.cpp:713
4 	XUL 	nsPopupBoxObject::HidePopup 	layout/xul/base/src/nsPopupBoxObject.cpp:93
5 	XUL 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
6 	XUL 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454
7 	XUL 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
8 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1386
9 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:5179
10 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1394
11 	libmozjs.dylib 	js_InternalInvoke 	js/src/jsinterp.cpp:1447
12 	libmozjs.dylib 	JS_CallFunctionValue 	js/src/jsapi.cpp:5187
13 	XUL 	nsJSContext::CallEventHandler 	dom/src/base/nsJSEnvironment.cpp:2035
14 	XUL 	nsGlobalWindow::RunTimeout 	
etc..

I think it is the same bug, I haven't been able to crash with this testcase on MacOS X 10.5. Probably the first 2 frames are munged, the other frames look the same.

The content of the iframe is this:
<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<popup id="auto" style="width: 100px; height: 100px;"/>

<script id="script" xmlns="http://www.w3.org/1999/xhtml"><![CDATA[
function doe() {
  document.getElementById('auto').openPopup();
  setTimeout(doe2, 40);
}

function doe2() {
window.frameElement.style.display = 'none';
  document.getElementById('auto').hidePopup();
}
setTimeout(doe, 100);

]]></script></window>

Comment 2

[Martijn Wargers (dead)]

Sorry, that last Marcia was me.

Comment 3

[timeless]

mw22: please file a new bug for that, it's different (simple null pointer).

Signature	nsViewManager::IsViewInserted(nsView*)
UUID	1c0b98a4-3c03-42ab-bcef-cf5492090709
Time 	2009-07-09 13:50:29.354813
Uptime	28
Last Crash	83 seconds before submission
Product	Firefox
Version	3.5
Build ID	20090624012136
Branch	1.9.1
OS	Mac OS X
OS Version	10.4.11 8S2167
CPU	x86
CPU Info	GenuineIntel family 6 model 14 stepping 8
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0x153b880
User Comments	Google toolbar new version
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	XUL 	nsViewManager::IsViewInserted 	view/src/nsView.h:124
1 	XUL 	nsViewManager::SetViewVisibility 	view/src/nsViewManager.cpp:1761
2 	XUL 	nsMenuPopupFrame::HidePopup 	layout/xul/base/src/nsMenuPopupFrame.cpp:668
3 	XUL 	nsXULPopupManager::HidePopupCallback 	layout/xul/base/src/nsXULPopupManager.cpp:761
4 	XUL 	nsXULPopupManager::FirePopupHidingEvent 	layout/xul/base/src/nsXULPopupManager.cpp:1096
5 	XUL 	nsXULPopupManager::HidePopup 	layout/xul/base/src/nsXULPopupManager.cpp:713
6 	XUL 	nsPopupBoxObject::HidePopup 	layout/xul/base/src/nsPopupBoxObject.cpp:93
7 	XUL 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
8 	XUL 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454
9 	XUL 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
10 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1386
11 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:5179

Comment 4

[Martijn Wargers (dead)]

It seems to me it is the same bug, because the crash only occurs on Mac OS 10.4, just like all the Google toolbar crashes. Also, with the unminimized testcases, I got the @ nsViewManager::IsViewInserted and @ nsViewManager::SetViewVisibility crashes.

Anyway, I think this should be fixed for the 1.9.1 branch, this didn't crash in Firefox 3, so it is a regression.
Like Marcia said, this is a topcrash on the Mac (only affecting Mac OS 10.4 in combination with the Google toolbar).
But I think this is responsible for the @ nsViewManager::IsViewInserted, the @ nsViewManager::SetViewVisibility and the @ nsMenuPopupFrame::HidePopup topcrashes.
They are resp. nr. 7, 10 and 18 on the topcrasher list for MacOS X. As further evidence that these are the same crashes, they all happen on MacOS X 10.4 only and comments often mention they were searching something within the Google toolbar.

Comment 5

[Martijn Wargers (dead)]

Also the nr. 35 crash seems related to this, the @ nsViewManager::ResizeView crash.

Comment 6

[Samuel Sidler (old account; do not CC)]

I'd love to mark this blocking for a 1.9.1.x release, but I'd love even more to have an owner who's at least investigating it. Does this belong in the hands of someone on the Mac team or someone on the Layout team?

Comment 7

[Martijn Wargers (dead)]

It seems to me this is a Mac widget issue.

Comment 8

[Samuel Sidler (old account; do not CC)]

Josh, can you own this or find someone to own it?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

This could be a Cocoa widget bug.  But it's at least as likely to be a
Layout bug (which for some reason is only triggered on 10.4).

I'd be willing to investigate this further ... at least far enough to
find out whether or not it's a Cocoa widgets bug (and then to fix it
if it is).

Unless Josh wants to do this :-)

Comment 10

[Samuel Sidler (old account; do not CC)]

Steven: Go for it (per Josh).

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb stack trace with debug symbols

I can easily reproduce this bug's crash with the testcase (attachment
387797).  No need to use/install the Google Toolbar.

I did have to wait 10-15 seconds for the crash to happen, though.

I tested on OS X 10.4.11 using an opt build with debug symbols made
from recent trunk (mozilla-central) code.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Source of testcase's embedded iframe

For reference, here's the source of the testcase's embedded iframe,
with its extended characters translated.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Here's the regression range for this bug:

firefox-2008-02-19-04-trunk
firefox-2008-02-20-04-trunk

I've currently no idea which patch in this range might have triggered
the problem.

I'm also not going to be able to spend any more time on this for at
least the next three weeks -- Josh wants me to work on other things,
and I'm also going on vacation for a week.

Comment 14

[Martijn Wargers (dead)]

Bonsai link for regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-19+04%3A00%3A00&maxdate=2008-02-20+05%3A00%3A00&cvsroot=%2Fcvsroot

Anyone else can/wants to take this?
This is a very bad bug, where basically everyone on MacOSX 10.4 crashes when they type something in the Google Toolbar text input.

Comment 15

[Samuel Sidler (old account; do not CC)]

roc: Can you please find an owner for this? (Knowing that it could move to a Cocoa widget person if the bug moves that way, of course...)

Comment 16

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

It might be related to the popup manager.

We need someone who has 10.4. I don't. I don't know who does. Neil, do you?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

For what it's worth:  Bug 503196 may turn out to be related to this bug, or even to have the same cause.  And that bug is reproducible on OS X 10.5.X.

Comment 18

[Neil Deakin]

I don't.

Looking at the regression range, there are a number of bugs that affect Cocoa. One bug in particular, 417124, adds non-Leopard-only code which redraws windows when popups are shown. The crashstack suggests a crash trying to retrieve the parent of a popup view. Could be coincidental, but would be a good place to start looking.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Another note:

I discovered that the following line in nsMenuPopupFrame::HidePopup()
can result in both viewManager and view being deleted before the next
line:

  viewManager->SetViewVisibility(view, nsViewVisibility_kHide);

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Another note:

AddRefing viewManager doesn't fix the crash :-)

I'll stop here.  Good luck!

Comment 21

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

I bet hiding the view is hiding the widget which somehow allows events to run which (because of the endless-reload script) is allowing the entire presentation to be torn down before we get to the next line.

Comment 22

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

Actually, looking at bug 417124 the patch there is clearly capable of causing this kind of crash. Triggering synchronous painting during a Show(PR_FALSE) can flush reflows and result in frame or even prescontext/viewmanager destruction.

Comment 23

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

I think we just need a new version of the patch for bug 417124 that invalidates the windows for asynchronous painting instead of synchronous painting.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

> I think we just need a new version of the patch for bug 417124 that
> invalidates the windows for asynchronous painting instead of
> synchronous painting.

You're right, Roc.  And thanks to both you and Neil for pointing out
things I'd missed, which make this bug quite easy to fix.

A tryserver build will follow in a few hours.

As best I can tell my patch doesn't regress bug 417124.  But I can't
reproduce that bug, even on OS X 10.4, using the trunk nightly from
the day before that patch landed (firefox-2008-02-19-04-trunk).  So
others will need to test more thoroughly.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #24:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla503391/bugzilla503391-macosx.dmg

Comment 26

[Marcia Knous [:marcia]]

Using the build in Comment 25,  Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090716 Minefield/3.6a1pre GTB5 I was not able to crash using either the STR in Comment 0 or using the testcase attached to the bug. Good work!

Comment 27

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

Steven, are you sure that setViewsNeedDisplay will actually cause repainting? You haven't marked any views as needing repainting, so I wonder if this will just cause it to look at its views and then stop without painting.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> Steven, are you sure that setViewsNeedDisplay will actually cause
> repainting?  You haven't marked any views as needing repainting, so
> I wonder if this will just cause it to look at its views and then
> stop without painting.

I'm not really sure :-)

I'm also not sure it'd be enough to mark the window's content view as
needing display.

That's why I noted that I can't reproduce bug 417124, and asked others
to test.

It may be that we simply no longer need to refresh the parent of a
popup that's getting hidden.

Comment 29

[Marcia Knous [:marcia]]

I was not able to reproduce bug 417124 following Jesse's STR in the bug. I tried a few other sites and the only thing I did notice was that on the weather.com site the focus ring does not behave the same way in Firefox as it does in Safari - but that may be another issuse.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Alternative fix (reverse main part of bug 417124 patch)

Here's an alternate version of my patch that simply reverses the main
part of the patch for bug 417124.  There seems to be a good chance
that it's no longer needed.

A tryserver build will follow in a few hours.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

I'm also going to test on the two branches ... though I may not get to that until next week.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my alternative patch from comment #30:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla503391-alt/bugzilla503391-alt-macosx.dmg

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to comment #3)
> it's different (simple null pointer).

executing null is different than simply dereferencing, and often indicates something scarier is going on.

Comment 34

[Josh Aas]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

I'm lacking a 10.4 partition at the moment but undoing the fix for 417124 is fine so long as somebody with a 10.4 partition has tested for regressing 417124. Even if it does regress 417124 we're probably better off fixing the crash than the visual polish.

Comment 35

[Marcia Knous [:marcia]]

Using the build in Comment 32, I did a little testing and did not see any issues with the following:

1. Testcase in the bug
2. Original STR
3. Issue with focus reported in Bug 417124

Comment 36

[Steven Michaud [:smichaud] (Retired)]

I've tested my alternative patch (attachment 389120 [details] [diff] [review]) with the STR for
bug 417124 (on OS X 10.4.11) on the trunk and both branches.  It
doesn't appear to regress bug 417124 ... at least in my tests.

Here are three tryserver builds (for the trunk and each branch):
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla503391-alt/bugzilla503391-alt-macosx.dmg
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla503391-alt-191branch/bugzilla503391-alt-191branch-macosx.dmg
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla503391-alt-190branch/[e-mail]

Marcia and Jesse, please test all three against the STR for bug 417124
(as well as this bug's STR).

Jesse, I'm asking you because you're the one who originally reported
bug 417124.  I'm not sure if you still have an OS X 10.4.11 partition
to test on.

Comment 37

[Jesse Ruderman]

I don't have 10.4 any more, but I think Waldo does.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

> but I think Waldo does

Where's Waldo? :-)

Comment 39

[Steven Michaud [:smichaud] (Retired)]

>> but I think Waldo does
>
> Where's Waldo? :-)

Actually, I think Marcia's results are enough.

I'm not good at evaluating UI bugs, so I didn't trust mine alone.

Comment 40

[Steven Michaud [:smichaud] (Retired)]

Marcia -- the trunk build from comment #36 is the same as that from comment #32, so you don't need to test that over again.

Comment 41

[Marcia Knous [:marcia]]

I tested the Shiretoko build listed in Comment 36, and all three items listed in Comment 35 pass as well.  Now on to the last build - 3 branches is exhausting!

Comment 42

[Marcia Knous [:marcia]]

Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.0.12pre) Gecko/2009072009 GranParadiso/3.0.12pre GTB5 (the tryserver build listed in Comment 36), the three items also pass. The trifecta is complete.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Thanks, Marcia!

I'll land this on the trunk.

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk (mozilla-central):
http://hg.mozilla.org/mozilla-central/rev/92678c089fb6

Comment 45

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

This bug's crash is the #11 topcrasher on the 1.9.1 branch (with 372
instances in the last week).  It also makes Google Toolbar completely
unusable on OS X 10.4.11.

Comment 46

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

For some reason this bug's crash doesn't appear in the 1.9.0 branch
topcrasher list, though it can easily be reproduced on the 1.9.0
branch using this bug's testcase (attachment 387797 [details]).  Possibly it's
more difficult to reproduce there in the Google Toolbar.

So it's less urgent to backport this patch to the 1.9.0 branch.  We
should probably wait until we see whether or not this patch causes any
UI regressions on the trunk or the 1.9.1 branch.

Comment 47

[Marcia Knous [:marcia]]

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre GTB5. I verified by checking the testcase, installing the Google toolbar and doing several searches, and checking the focus issue in Bug 417124.

Comment 48

[Jeff Walden [:Waldo]]

I do indeed run 10.4 still, as I think I shouldn't have to upgrade my OS for at least the length of time it's under warranty (still have another year left, but Gecko development may force my hand, unfortunately).

I'm not sure if I'm needed based on the comments here, assuming no for the moment; if so, please clearly point out the STR in a comment, because they're not obvious at a glance.

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

a1912=beltzner

Comment 50

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e6221c785010

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Oddly, I'm not able to add the 'fixed1.9.1.2' keyword.

Comment 52

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #51)
> Oddly, I'm not able to add the 'fixed1.9.1.2' keyword.

It's now a flag at status1.9.1.

Comment 53

[Steven Michaud [:smichaud] (Retired)]

> It's now a flag at status1.9.1.

Oh, OK.  Sorry.  I'll eventually learn the new flags :-)

Comment 54

[Daniel Veditz [:dveditz]]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

Approved for 1.9.0.13, a=dveditz for release-drivers

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

Someone else will need to land this on the 1.9.0 branch.

I'm away for the next week.

Comment 56

[Marcia Knous [:marcia]]

Verified on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 GTB5. Verified using the steps in Comment 47. Adding keyword.

Comment 57

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

Landed on the 1.9.0 branch:

Checking in widget/src/cocoa/nsCocoaWindow.mm;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v  <--  nsCocoaWindow.mm
new revision: 1.153; previous revision: 1.152
done

Comment 58

[Samuel Sidler (old account; do not CC)]

Please use the appropriate fixed keyword for the approval flag showing. In this case, we firedrilled 1.9.0.13, so we renamed the flag.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

> we firedrilled 1.9.0.13

Oops, I missed that.

Comment 60

[Al Billings [:abillings - ex-MoCo]]

Marcia, can you verify that this is fixed in a 1.9.0 nightly on 10.4? I don't have 10.4 available.