Closed Bug 642338 Opened 13 years ago Closed 13 years ago

crash [@ nsBarProp::GetVisibleByFlag(int*, unsigned int)]

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla5
Tracking Flags:
Tracking Status
firefox5 + fixed
blocking2.0 --- -
status2.0 --- wontfix
blocking1.9.2 --- .18+
status1.9.2 --- .18-fixed
blocking1.9.1 --- -
status1.9.1 --- .20-fixed

People

(Reporter: kbrosnan, Assigned: justin.lebar+bug)

References

Details

(Keywords: crash, regression, Whiteboard: [sg:critical?] [qa-examined-192] [qa-ntd-192])

Crash Data

Attachments

(3 files, 2 obsolete files)

Fix? v3
19.46 KB, patch
johnath
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Fix for 1.9.2
19.60 KB, patch
dveditz
: approval1.9.2.18+
Details | Diff | Splinter Review
Fix for 1.9.1
19.63 KB, patch
dveditz
: approval1.9.1.20+
Details | Diff | Splinter Review
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
0 	xul.dll 	nsBarProp::GetVisibleByFlag 	dom/base/nsBarProps.cpp:93
1 	xul.dll 	nsStatusbarProp::GetVisible 	dom/base/nsBarProps.cpp:252
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
3 	xul.dll 	XPC_WN_GetterSetter 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1663
4 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:703
5 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
6 	mozjs.dll 	js::ExternalGetOrSet 	js/src/jsinterp.cpp:903
7 	mozjs.dll 	js::Shape::get 	js/src/jsscopeinlines.h:249
8 	mozjs.dll 	js_GetPropertyHelper 	js/src/jsobj.cpp:5463
9 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4215
10 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:653
11 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:740
12 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
13 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5173
14 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1914
15 	xul.dll 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:228
16 	xul.dll 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1127
17 	xul.dll 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:1224
18 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:628
blocking2.0: --- → ?
Group: core-security
I don't know why the crash happens, but the stack look suspicious.
I can't reproduce this.
I don't see why we'd block on this, and no rationale was given.
blocking2.0: ? → -
Agreed w/comment 2, though may not be trigerable from content if it involves nsBarProp?  Kevin: can you repro? What about in safe-mode? a couple of the addons listed in that crash stack do toolbar things.
Keywords: testcase-wanted
Could be bad, if it's crashing because mBrowserChrome is pointing at a deleted object. If we happened to get some readable memory we'd next try to call a function pointer on it.
Whiteboard: [sg:critical?]
checking --- nsBarProp::GetVisibleByFlag.int...unsigned.int.
20110316-crashdata.csv
found in: 3.6.15 4.0 3.6.13 3.6.10 4.0b10 3.6.11 3.6 3.5.17 3.5.13
release total-crashes
              nsBarProp::GetVisibleByFlag.int...unsigned.int. crashes
                         pct.
all      367839    38    0.000103306
3.6.15    158092    21    0.000132834
4.0    90510    7    7.73395e-05
3.6.13    29272    3    0.000102487
3.6.10    3724    2    0.000537057
4.0b10    3737    1    0.000267594
3.6.11    659    1    0.00151745
3.6    4182    1    0.00023912
3.5.17    7445    1    0.000134318
3.5.13    187    1    0.00534759


os breakdown
nsBarProp::GetVisibleByFlag.int...unsigned.int.Total 38
Win5.1  0.24
Win6.0  0.16
Win6.1  0.58


omains of sites
   9 http://www.hulu.com
   1 http://www.hulu.com/watch/224051/the-biggest-loser-week-11
   1 http://www.hulu.com/watch/223691/glee-gleewind-sexy#s-p1-sr-i1
   1 http://www.hulu.com/watch/222879/traffic-light-no-good-deed
   1
http://www.hulu.com/watch/222454/pretty-little-liars-monsters-in-the-end#play-queued-show-by-original_premiere_date-asc#continuous_play=on
   1
http://www.hulu.com/watch/219844/the-simpsons-a-midsummers-nice-dream#s-p1-so-i0
   1 http://www.hulu.com/watch/219040/pretty-little-liars-a-person-of-interest
   1
http://www.hulu.com/watch/215300/modern-marvels-the-potato#continuous_play=on
   1 http://www.hulu.com/watch/196994/sons-of-anarchy-the-push#s-p1-so-i0
   1
http://www.hulu.com/watch/168025/what-would-you-do-tue-mar-10-2009#s-p5-so-i0

   5 http://maps.google.com

   4 http://maps.google.de
   2 javascript:false;//
   2 http://www.youtube.com
   2 http://apps.facebook.com
   1 http://apps.facebook.com/frontierville/index.php
   1 http://apps.facebook.com/frontierville/

   1 http://www.meinvz.net
   1 http://www.flickr.com
   1 http://www.facebook.com
   1 http://www.daserste.de
   1 http://ups.surveyrouter.com
   1 http://shinywhite.dk
   1 http://server1.puschelfarm.de
   1 http://nk.pl
   1 http://maps.google.fr
   1 http://maps.google.co.in
   1 http://maps.google.co.id
   1 http://classic.wunderground.com
http://classic.wunderground.com/cgi-bin/findweather/getForecast?query=01532&MR=1
I could not reproduce it immediately after the crash will check again over the weekend.
Kevin: Were you ever able to reproduce this?

(In reply to comment #8)
> I could not reproduce it immediately after the crash will check again over the
> weekend.
I was not. I go to the site I crashed on quite often but have not crashed since the report.
Justin says he can have a look at the memory management code around nsBarProp, but given the lack of a reproducable testcase that may or may not revile anything here...
Assignee: nobody → justin.lebar+bug
I think the crashing JS may be in nsSessionStore.js:

    var hidden = WINDOW_HIDEABLE_FEATURES.filter(function(aItem) {
      return aWindow[aItem] && !aWindow[aItem].visible;
    });

...there just aren't too many places where we touch this property, afacit.

Still not sure what the problem is, though.
If this is indeed where we're dying, the fact that the fault is on the visible call indicates that the window probably wasn't destroyed, no?  That would be an interesting state of affairs...
Attached patch Fix? v1 (obsolete) — — Splinter Review 
Looking more closely at the stack trace, this appears to be a use-after-free of nsBarProp::mBrowserChrome, since this frame

  0     xul.dll     nsBarProp::GetVisibleByFlag     dom/base/nsBarProps.cpp:93

corresponds to this code

  NS_ENSURE_SUCCESS(mBrowserChrome->GetChromeFlags(&chromeFlags),
                    NS_ERROR_FAILURE);

and mBrowserChrome is a raw pointer.

It's not clear to me how we get into the situation where mBrowserChrome is dangling, but it's easy enough to stick a weak reference in there.  We could push to m-c and see if this fixes the crash.
Attachment #527660 - Flags: review?(Olli.Pettay)
Comment on attachment 527660 [details] [diff] [review]
Fix? v1

> protected:
>-  // Weak Reference
>-  nsIWebBrowserChrome* mBrowserChrome;
>+  // Weak Reference to nsIWebBrowserChrome.  I suspect using a nsWeakPtr here
>+  // as opposed to a raw nsIWebBrowserChrome may prevent a crash (bug 642338),
>+  // but it's not clear how we could get a stale pointer in here in the first
>+  // place.
This comment is way too clear indication of an sg:crit bug.

>+  nsWeakPtr mBrowserChrome;


>+++ b/xpfe/appshell/src/nsContentTreeOwner.cpp
>@@ -125,16 +125,17 @@ NS_INTERFACE_MAP_BEGIN(nsContentTreeOwne
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDocShellTreeOwner)
>    NS_INTERFACE_MAP_ENTRY(nsIDocShellTreeOwner)
>    NS_INTERFACE_MAP_ENTRY(nsIBaseWindow)
>    NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChrome)
>    NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChrome2)
>    NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChrome3)
>    NS_INTERFACE_MAP_ENTRY(nsIInterfaceRequestor)
>    NS_INTERFACE_MAP_ENTRY(nsIWindowProvider)
>+   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
This shows a problem :( Need to implement nsISupportsWeakReference
in all the relevant places.
But I guess there aren't too many implementations for nsIDocsShellTreeOwner.
nsDocShellTreeOwner and nsChromeTreeOwner do already implement weakref.
Have you made sure other implementations (if there are any other) implement weakref?

And the stale pointer can be there if the pointer isn't cleared properly.
Are we not calling nsGlobalWindow::SetDocShell() always?
> Have you made sure other implementations (if there are any other) implement
> weakref?

I looked through the results of

  hg locate *.h | xargs grep 'public nsIWebBrowserChrome'

and checked that all the resulting classes implement nsSupportsWeakReference.  Are there other classes I need to check?
Actually, would it better to move the members of nsScrollbarsProp to nsBarProp
and then get browserchrome from window's docshell.
(In reply to comment #17)
> Actually, would it better to move the members of nsScrollbarsProp to nsBarProp
> and then get browserchrome from window's docshell.

Not sure if that was a question.  :)

nsScrollbarsProp is the only one which gets a reference to the global window in its constructor, so as it is, I think it makes sense to keep the global window pointers on nsScrollbarsProp rather than move them to nsBarProp.

Is there a good reason not to leave things as they are?
IMO, all the "bars" are properties of nsGlobalWindow and should get data from
there. And nsGlobalWindow does already implement weakref and has
GetWebBrowserChrome(...), so why not just reuse that all.
No need to add assumption that nsIDocShellTreeOwner implementation
must implement also weakref.
I wonder why I didn't make that change when I was fixing
a security bug almost like this one :/
Attached patch Fix? v2 (obsolete) — — Splinter Review 
Fair enough!  How does this look to you?
Attachment #527660 - Attachment is obsolete: true
Attachment #527660 - Flags: review?(Olli.Pettay)
Attachment #527781 - Flags: review?(Olli.Pettay)
Comment on attachment 527781 [details] [diff] [review]
Fix? v2

/me likes.
And btw, this should fix *Bar* to work when a tab is moved to another window.


>+// Script "scrollbars" object
>+class nsScrollbarsProp : public nsBarProp {
{ should be in the next line


I don't see reason for explicit, but feel free to keep it.


Do we have any tests for *bar*? If not, please add some.
Attachment #527781 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #22)
> Do we have any tests for *bar*? If not, please add some.

Short of testing that window.statusbar is defined (and, somewhat surprisingly, I see nothing which checks this), what do you think needs testing?
perhaps you could add a test which opens two windows, one
with menubar, one without and check that window.menubar.visible has the
correct value.
Same with others, although I don't know if statusbar.visible is ever
true, and I have no idea what personalbar is :)
Attached patch Fix? v3 — — Splinter Review 
Okay, added tests.

How do we want to manage landing this?  Should I just push to m-c and watch crashstats to see if the problem goes away, or do we want to land on branches immediately to minimize exposure if someone figures out how to exploit this?
Attachment #527781 - Attachment is obsolete: true
Fixed in trunk.  Requesting approval for branches.
...well, first I should make sure that the patch applies against the branch!

http://hg.mozilla.org/mozilla-central/rev/223eac79b04e
Comment on attachment 527842 [details] [diff] [review]
Fix? v3

This applies with only trivial modifications to mozilla-aurora and mozilla-2.0, so requesting approval for those branches.  I'll post patches for mozilla-1.9.2 and 1.9.1.
Attachment #527842 - Flags: approval2.0?
Attachment #527842 - Flags: approval-mozilla-aurora?
Attached patch Fix for 1.9.2 — — Splinter Review 
The 1.9.2 fix is the same as the previous fixes; it just needs a different patch because of whitespace conflicts in nsGlobalWindow.cpp.
Attachment #528770 - Flags: approval1.9.2.18?
Attached patch Fix for 1.9.1 — — Splinter Review 
Again, only mechanical changes wrt the other patches.  This time, some files moved around.
Attachment #528772 - Flags: approval1.9.1.20?
blocking1.9.1: --- → -
blocking1.9.2: --- → .18+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: --- → +
Last crash on trunk was April 27, when I checked this in.  So it looks like we did fix the problem on trunk.
Depends on: 653996
Comment on attachment 528770 [details] [diff] [review]
Fix for 1.9.2

Approved for 1.9.2.18, a=dveditz for release-drivers
Attachment #528770 - Flags: approval1.9.2.18? → approval1.9.2.18+
Comment on attachment 528772 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.20, a=dveditz for release-drivers

Mozilla isn't planning on shipping a 3.5.20 so you don't actually need to check this in. Maybe the distros will want it though.
Attachment #528772 - Flags: approval1.9.1.20? → approval1.9.1.20+
Attachment #527842 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Checked in to

aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/15a401f92732
1.9.2:  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3c3409c8fb1f
1.9.1:  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bee62d2796d5

But not 2.0, since the bug was marked status-2.0: wontfix.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #527842 - Flags: approval2.0?
Comment on attachment 527842 [details] [diff] [review]
Fix? v3

>+  // You can't turn these off even if you try, so check that they're true.
>+  is(w.locationbar.visible, true, "locationbar");
This one depends on a pref in firefox.js, so the test only passes on Firefox.

>+  is(w.statusbar.visible, true, "statusbar");
This one is OK because it's in all.js which all apps share.
In bug 653996 comment 3, Serge suggests that you might want to change the SM behavior to match Firefox's.  Is this something you want to pursue?

If not, I'll review a patch to modify the test so it checks the pref before the is() call.
(In reply to comment #36)

> In bug 653996 comment 3, Serge suggests that you might want to change the SM
> behavior to match Firefox's.  Is this something you want to pursue?

Let's see about that in bug 653996.

Unrelated to SeaMonkey decision, a Core test should not rely on a Firefox/application specific preference: this test should be fixed anyway.

> If not, I'll review a patch to modify the test so it checks the pref before
> the is() call.

To be explicit, is() check is fine in both cases, but its expected value should simply depend on "dom.disable_window_open_feature.location"(!?) preference value.
(In reply to comment #36)
> In bug 653996 comment 3, Serge suggests that you might want to change the SM
> behavior to match Firefox's.  Is this something you want to pursue?
I don't mind if the powers that be agree to move the preference from firefox.js to all.js, but I don't see the point of duplicating it in our prefs.
I've filed bug 661132 on fixing the test.
Depends on: 661132
(In reply to comment #34)
> aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/15a401f92732

+ "status-firefox5 	affected 	fixed"

But
http://mxr.mozilla.org/mozilla-beta/find?text=&string=test_window_bar.html
doesn't exist atm.
What is actual status wrt mozilla 5-6-7?
Nobody seemed to know about the MXR issue on IRC, so I filed bug 662380.
Flags: in-testsuite+
Target Milestone: --- → mozilla5
Did we ever find any reliable STR for this?
Whiteboard: [sg:critical?] → [sg:critical?] [qa-examined-192]
(In reply to comment #43)
> Did we ever find any reliable STR for this?

No, I don't think so.
Crash Signature: [@ nsBarProp::GetVisibleByFlag(int*, unsigned int)]
Whiteboard: [sg:critical?] [qa-examined-192] → [sg:critical?] [qa-examined-192] [qa-needs-STR]
Whiteboard: [sg:critical?] [qa-examined-192] [qa-needs-STR] → [sg:critical?] [qa-examined-192] [qa-ntd-192]
Group: core-security
Component: DOM → DOM: Core & HTML

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Flags: needinfo?(jstutte)
Restrict Comments: true
Flags: needinfo?(odadingmangoleeh)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: