Last Comment Bug 642338 - crash [@ nsBarProp::GetVisibleByFlag(int*, unsigned int)]
: crash [@ nsBarProp::GetVisibleByFlag(int*, unsigned int)]
Status: RESOLVED FIXED
[sg:critical?] [qa-examined-192] [qa-...
: crash
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: mozilla5
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 653996 661132
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-16 17:53 PDT by Kevin Brosnan [:kbrosnan]
Modified: 2015-10-16 11:37 PDT (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
-
wontfix
.18+
.18-fixed
-
.20-fixed


Attachments
Fix? v1 (5.80 KB, patch)
2011-04-21 15:15 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Fix? v2 (16.37 KB, patch)
2011-04-22 09:05 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review
Fix? v3 (19.46 KB, patch)
2011-04-22 13:11 PDT, Justin Lebar (not reading bugmail)
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Fix for 1.9.2 (19.60 KB, patch)
2011-04-27 18:21 PDT, Justin Lebar (not reading bugmail)
dveditz: approval1.9.2.18+
Details | Diff | Splinter Review
Fix for 1.9.1 (19.63 KB, patch)
2011-04-27 18:46 PDT, Justin Lebar (not reading bugmail)
dveditz: approval1.9.1.20+
Details | Diff | Splinter Review

Comment 1 Kevin Brosnan [:kbrosnan] 2011-03-16 17:54:39 PDT
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
Comment 2 Olli Pettay [:smaug] 2011-03-17 10:14:06 PDT
I don't know why the crash happens, but the stack look suspicious.
Comment 3 Olli Pettay [:smaug] 2011-03-17 10:41:31 PDT
I can't reproduce this.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-17 11:35:05 PDT
I don't see why we'd block on this, and no rationale was given.
Comment 5 Daniel Veditz [:dveditz] 2011-03-17 13:31:37 PDT
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.
Comment 6 Daniel Veditz [:dveditz] 2011-03-17 13:33:57 PDT
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.
Comment 7 chris hofmann 2011-03-17 13:41:12 PDT
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
Comment 8 Kevin Brosnan [:kbrosnan] 2011-03-17 23:05:26 PDT
I could not reproduce it immediately after the crash will check again over the weekend.
Comment 9 Marcia Knous [:marcia - use ni] 2011-03-31 13:58:41 PDT
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.
Comment 10 Kevin Brosnan [:kbrosnan] 2011-04-01 11:27:18 PDT
I was not. I go to the site I crashed on quite often but have not crashed since the report.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2011-04-14 13:51:34 PDT
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...
Comment 12 Justin Lebar (not reading bugmail) 2011-04-20 12:19:58 PDT
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.
Comment 13 Justin Lebar (not reading bugmail) 2011-04-21 12:31:22 PDT
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...
Comment 14 Justin Lebar (not reading bugmail) 2011-04-21 15:15:13 PDT
Created attachment 527660 [details] [diff] [review]
Fix? v1

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.
Comment 15 Olli Pettay [:smaug] 2011-04-21 15:29:40 PDT
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?
Comment 16 Justin Lebar (not reading bugmail) 2011-04-21 15:34:26 PDT
> 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?
Comment 17 Olli Pettay [:smaug] 2011-04-21 23:49:27 PDT
Actually, would it better to move the members of nsScrollbarsProp to nsBarProp
and then get browserchrome from window's docshell.
Comment 18 Justin Lebar (not reading bugmail) 2011-04-22 08:15:52 PDT
(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?
Comment 19 Olli Pettay [:smaug] 2011-04-22 08:20:35 PDT
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.
Comment 20 Olli Pettay [:smaug] 2011-04-22 08:21:46 PDT
I wonder why I didn't make that change when I was fixing
a security bug almost like this one :/
Comment 21 Justin Lebar (not reading bugmail) 2011-04-22 09:05:28 PDT
Created attachment 527781 [details] [diff] [review]
Fix? v2

Fair enough!  How does this look to you?
Comment 22 Olli Pettay [:smaug] 2011-04-22 09:35:02 PDT
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.
Comment 23 Justin Lebar (not reading bugmail) 2011-04-22 11:25:45 PDT
(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?
Comment 24 Olli Pettay [:smaug] 2011-04-22 11:44:24 PDT
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 :)
Comment 25 Justin Lebar (not reading bugmail) 2011-04-22 13:11:28 PDT
Created attachment 527842 [details] [diff] [review]
Fix? v3

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?
Comment 26 Justin Lebar (not reading bugmail) 2011-04-27 16:22:25 PDT
Fixed in trunk.  Requesting approval for branches.
Comment 27 Justin Lebar (not reading bugmail) 2011-04-27 16:23:04 PDT
...well, first I should make sure that the patch applies against the branch!

http://hg.mozilla.org/mozilla-central/rev/223eac79b04e
Comment 28 Justin Lebar (not reading bugmail) 2011-04-27 18:13:56 PDT
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.
Comment 29 Justin Lebar (not reading bugmail) 2011-04-27 18:21:11 PDT
Created attachment 528770 [details] [diff] [review]
Fix for 1.9.2

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.
Comment 30 Justin Lebar (not reading bugmail) 2011-04-27 18:46:38 PDT
Created attachment 528772 [details] [diff] [review]
Fix for 1.9.1

Again, only mechanical changes wrt the other patches.  This time, some files moved around.
Comment 31 Justin Lebar (not reading bugmail) 2011-04-30 12:27:09 PDT
Last crash on trunk was April 27, when I checked this in.  So it looks like we did fix the problem on trunk.
Comment 32 Daniel Veditz [:dveditz] 2011-05-04 10:58:44 PDT
Comment on attachment 528770 [details] [diff] [review]
Fix for 1.9.2

Approved for 1.9.2.18, a=dveditz for release-drivers
Comment 33 Daniel Veditz [:dveditz] 2011-05-04 10:59:45 PDT
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.
Comment 34 Justin Lebar (not reading bugmail) 2011-05-04 13:40:11 PDT
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.
Comment 35 neil@parkwaycc.co.uk 2011-05-31 04:09:05 PDT
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.
Comment 36 Justin Lebar (not reading bugmail) 2011-05-31 05:54:48 PDT
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.
Comment 37 Serge Gautherie (:sgautherie) 2011-05-31 17:07:04 PDT
(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.
Comment 38 neil@parkwaycc.co.uk 2011-06-01 01:53:30 PDT
(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.
Comment 39 Justin Lebar (not reading bugmail) 2011-06-01 06:12:31 PDT
I've filed bug 661132 on fixing the test.
Comment 40 Serge Gautherie (:sgautherie) 2011-06-06 13:23:29 PDT
(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?
Comment 42 Justin Lebar (not reading bugmail) 2011-06-06 13:57:55 PDT
Nobody seemed to know about the MXR issue on IRC, so I filed bug 662380.
Comment 43 Al Billings [:abillings] 2011-06-07 16:41:12 PDT
Did we ever find any reliable STR for this?
Comment 44 Justin Lebar (not reading bugmail) 2011-06-07 17:51:46 PDT
(In reply to comment #43)
> Did we ever find any reliable STR for this?

No, I don't think so.

Note You need to log in before you can comment on or make changes to this bug.