The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 5

Status

()

Core
DOM
--
critical
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Depends on: 1 bug, {crash})

Trunk
mozilla5
x86
Windows 7
crash
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5+ fixed, blocking2.0 -, status2.0 wontfix, blocking1.9.2 .18+, status1.9.2 .18-fixed, blocking1.9.1 -, status1.9.1 .20-fixed)

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
This bug was filed from the Socorro interface and is 
report bp-fbfefcf6-c046-436b-97e5-363552110316 .
============================================================= 

33 crashes on the 4.0 RC https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A4.0&range_value=1&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=nsBarProp%3A%3AGetVisibleByFlag%28int*%2C%20unsigned%20int%29

Opened http://www.weatherunderground.com/cgi-bin/findweather/getForecast?query=02901 then crashed shortly after the page load was done.
(Reporter)

Updated

6 years ago
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Updated

6 years ago
blocking2.0: --- → ?

Updated

6 years ago
Group: core-security

Comment 2

6 years ago
I don't know why the crash happens, but the stack look suspicious.

Comment 3

6 years ago
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?]

Comment 7

6 years ago
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
(Reporter)

Comment 8

6 years ago
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.
(Reporter)

Comment 10

6 years ago
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
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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...
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 16

6 years ago
> 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.
(Assignee)

Comment 18

6 years ago
(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 :/
(Assignee)

Comment 21

6 years ago
Created attachment 527781 [details] [diff] [review]
Fix? v2

Fair enough!  How does this look to you?
(Assignee)

Updated

6 years ago
Attachment #527660 - Attachment is obsolete: true
Attachment #527660 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 23

6 years ago
(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 :)
(Assignee)

Comment 25

6 years ago
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?
(Assignee)

Updated

6 years ago
Attachment #527781 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Fixed in trunk.  Requesting approval for branches.
(Assignee)

Comment 27

6 years ago
...well, first I should make sure that the patch applies against the branch!

http://hg.mozilla.org/mozilla-central/rev/223eac79b04e
(Assignee)

Comment 28

6 years ago
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?
(Assignee)

Comment 29

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #528770 - Flags: approval1.9.2.18?
(Assignee)

Comment 30

6 years ago
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.
(Assignee)

Updated

6 years ago
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: --- → +
(Assignee)

Comment 31

6 years ago
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
status2.0: wanted → wontfix
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+
(Assignee)

Comment 34

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #527842 - Flags: approval2.0?
(Assignee)

Updated

6 years ago
status1.9.1: wanted → .20-fixed
status1.9.2: wanted → .18-fixed
status-firefox5: affected → fixed
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.
(Assignee)

Comment 36

6 years ago
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.
(Assignee)

Comment 39

6 years ago
I've filed bug 661132 on fixing the test.
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 41

6 years ago
I think mxr is wrong.

Compare

http://hg.mozilla.org/releases/mozilla-beta/file/tip/dom/base/nsBarProps.cpp

to

http://mxr.mozilla.org/mozilla-beta/source/dom/base/nsBarProps.cpp
(Assignee)

Comment 42

6 years ago
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]
(Assignee)

Comment 44

6 years ago
(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
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.