Closed Bug 503391 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Widget: Cocoa, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .2-fixed

People

(Reporter: marcia, Assigned: smichaud)

References

Details

(6 keywords, Whiteboard: [sg:critical?] not a crash in 1.9.0)

Crash Data

Attachments

(4 files, 1 obsolete file)

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.
Attached file 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>
Sorry, that last Marcia was me.
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
Component: Widget: Mac → Layout: View Rendering
QA Contact: mac → layout.view-rendering
Summary: [Mac 10.4] Crash in [nsViewManager::IsViewInserted(nsView*)] using Google Toolbar → [Mac 10.4] Crash in [@ nsViewManager::IsViewInserted(nsView*)] using Google Toolbar
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.
Flags: wanted1.9.2?
Flags: wanted1.9.1.x?
Flags: blocking1.9.2?
Flags: blocking1.9.1.1?
Keywords: regression, testcase
Also the nr. 35 crash seems related to this, the @ nsViewManager::ResizeView crash.
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?
Flags: wanted1.9.1.x? → wanted1.9.1.x+
It seems to me this is a Mac widget issue.
Josh, can you own this or find someone to own it?
Assignee: nobody → joshmoz
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 :-)
Steven: Go for it (per Josh).
Assignee: joshmoz → smichaud
blocking1.9.1: --- → needed
Flags: blocking1.9.1.1? → blocking1.9.1.1-
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.
For reference, here's the source of the testcase's embedded iframe,
with its extended characters translated.
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.
Assignee: smichaud → nobody
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.
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...)
Assignee: nobody → roc
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?
Assignee: roc → administration
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.
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.
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);
Another note:

AddRefing viewManager doesn't fix the crash :-)

I'll stop here.  Good luck!
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.
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.
No longer blocks: 417124
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.
Assignee: administration → nobody
Component: Layout: View Rendering → Widget: Cocoa
QA Contact: layout.view-rendering → cocoa
Assignee: nobody → smichaud
Attached patch Fix (obsolete) — Splinter Review
> 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.
Attachment #388927 - Flags: review?(joshmoz)
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!
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.
> 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.
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.
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.
Attachment #389120 - Flags: review?(joshmoz)
I'm also going to test on the two branches ... though I may not get to that until next week.
(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.
Group: core-security
Whiteboard: [sg:critical?]
Flags: wanted1.9.0.x-
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.
Attachment #389120 - Flags: review?(joshmoz) → review+
Attachment #388927 - Flags: review?(joshmoz)
Attachment #388927 - Attachment is obsolete: true
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
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/smichaud@pobox.com-bugzilla503391-alt/bugzilla503391-alt-macosx.dmg
https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-bugzilla503391-alt-191branch/bugzilla503391-alt-191branch-macosx.dmg
https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-bugzilla503391-alt-190branch/smichaud@pobox.com-bugzilla503391-alt-190branch-firefox-try-macosx.dmg

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.
I don't have 10.4 any more, but I think Waldo does.
> but I think Waldo does

Where's Waldo? :-)
>> 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.
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.
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!
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.
Thanks, Marcia!

I'll land this on the trunk.
Landed on trunk (mozilla-central):
http://hg.mozilla.org/mozilla-central/rev/92678c089fb6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #389120 - Flags: approval1.9.1.2?
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 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.
Attachment #389120 - Flags: approval1.9.0.13?
Keywords: topcrash
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.
Status: RESOLVED → VERIFIED
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 on attachment 389120 [details] [diff] [review]
Alternative fix (reverse main part of bug 417124 patch)

a1912=beltzner
Attachment #389120 - Flags: approval1.9.1.2? → approval1.9.1.2+
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
Oddly, I'm not able to add the 'fixed1.9.1.2' keyword.
(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.
> It's now a flag at status1.9.1.

Oh, OK.  Sorry.  I'll eventually learn the new flags :-)
Whiteboard: [sg:critical?] → [sg:critical?] not a crash in 1.9.0
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
Attachment #389120 - Flags: approval1.9.0.13? → approval1.9.0.13+
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.
Whiteboard: [sg:critical?] not a crash in 1.9.0 → [sg:critical?] not a crash in 1.9.0 [needs landing on 1.9.0.13]
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.
Keywords: verified1.9.1
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
Whiteboard: [sg:critical?] not a crash in 1.9.0 [needs landing on 1.9.0.13] → [sg:critical?] not a crash in 1.9.0
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.
> we firedrilled 1.9.0.13

Oops, I missed that.
Marcia, can you verify that this is fixed in a 1.9.0 nightly on 10.4? I don't have 10.4 available.
Group: core-security
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
blocking1.9.1: needed → ---
Crash Signature: [@ nsViewManager::IsViewInserted(nsView*)]
You need to log in before you can comment on or make changes to this bug.