345529 - crash removing an observer during an nsPref:changed notification [@ pref_DoCallback] (node is 0xdddddddd)

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

crash in pref_DoCallback() (node is 0xdddddddd) when I use about:config to change the hidden browser.tabs.closeButtons pref

this happend to me on the trunk a couple times, but it's not always reproducable.

to reproduce:

1)  I had multiple windows with mutliple tabs per window
2)  about:config in one
3)  create new integer pref (hidden browser.tabs.closeButtons)
4)  set to 1

sometimes, boom.

other times, when I went to change the pref from 1 to 0 (or 0 to 1?), boom.

but not always.

node points to freed memory:

-		node	0xdddddddd {domain=??? func=??? data=??? ...}	CallbackNode *

Here's the stack:

>	xppref32.dll!pref_DoCallback(const char * changed_pref=0x07aa4300)  Line 875 + 0x3 bytes	C++
 	xppref32.dll!pref_HashPref(const char * key=0x07aa4300, PrefValue value={...}, PrefType type=PREF_INT, int set_default=0)  Line 770 + 0x9 bytes	C++
 	xppref32.dll!PREF_SetIntPref(const char * pref_name=0x07aa4300, int value=0, int set_default=0)  Line 299 + 0x13 bytes	C++
 	xppref32.dll!nsPrefBranch::SetIntPref(const char * aPrefName=0x07aa4300, int aValue=0)  Line 223 + 0x14 bytes	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x0000000a, unsigned int methodIndex=2, unsigned int paramCount=1235872, nsXPTCVariant * params=0x00c5ee98)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=10)  Line 2162 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2162 + 0x1e bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x07cce8e0, JSObject * obj=0x0504f088, unsigned int argc=2, long * argv=0x07e24578, long * vp=0x0012de70)  Line 1450 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x07cce8e0, unsigned int argc=2, unsigned int flags=0)  Line 1349 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x07cce8e0, unsigned char * pc=0x053628bc, long * result=0x0012e9fc)  Line 4084 + 0xf bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x07cce8e0, unsigned int argc=1, unsigned int flags=2)  Line 1368 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x07cce8e0, JSObject * obj=0x07e19d80, long fval=132230008, unsigned int flags=0, unsigned int argc=1, long * argv=0x07e243d8, long * rval=0x0012eb50)  Line 1447 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x07cce8e0, JSObject * obj=0x07e19d80, long fval=132230008, unsigned int argc=1, long * argv=0x07e243d8, long * rval=0x0012eb50)  Line 4385 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x07b32818, void * aScope=0x03e727d0, void * aHandler=0x07e1ab78, nsIArray * aargv=0x07e214e0, nsIVariant * * arv=0x0012ecc0)  Line 1731 + 0x21 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x07d10754)  Line 209 + 0x62 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x07cfb508, nsIDOMEventListener * aListener=0x07cfb460, nsIDOMEvent * aDOMEvent=0x07d10754, nsISupports * aCurrentTarget=0x07b32818, unsigned int aSubType=8, unsigned int aPhaseFlags=6)  Line 1648 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x02cd5520, nsEvent * aEvent=0x0012f058, nsIDOMEvent * * aDOMEvent=0x0012ef88, nsISupports * aCurrentTarget=0x07b32818, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012ef8c)  Line 1752	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 356	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 433	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x07b32818, nsPresContext * aPresContext=0x02cd5520, nsEvent * aEvent=0x0012f058, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f054, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0)  Line 639 + 0x12 bytes	C++
 	gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x07b32818, nsEvent * aEvent=0x0012f058, nsEventStatus * aStatus=0x0012f054)  Line 6321 + 0x1e bytes	C++
 	gklayout.dll!nsMenuFrame::Execute(nsGUIEvent * aEvent=0x0012f594)  Line 1659	C++
 	gklayout.dll!nsMenuFrame::HandleEvent(nsPresContext * aPresContext=0x02cd5520, nsGUIEvent * aEvent=0x0012f594, nsEventStatus * aEventStatus=0x0012f1a4)  Line 451 + 0xc bytes	C++
 	gklayout.dll!nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor & aVisitor={...})  Line 1498	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x0012f25c)  Line 481	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x07b32818, nsPresContext * aPresContext=0x02cd5520, nsEvent * aEvent=0x0012f594, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f358, nsDispatchingCallback * aCallback=0x0012f25c, int aTargetIsChromeHandler=0)  Line 639 + 0x12 bytes	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f594, nsIView * aView=0x07d101d0, nsEventStatus * aStatus=0x0012f358)  Line 6278 + 0x2b bytes	C++
 	gklayout.dll!PresShell::HandlePositionedEvent(nsIView * aView=0x07d101d0, nsIFrame * aTargetFrame=0x07e0d688, nsGUIEvent * aEvent=0x0012f594, nsEventStatus * aEventStatus=0x0012f358)  Line 6153 + 0x14 bytes	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x07d101d0, nsGUIEvent * aEvent=0x0012f594, nsEventStatus * aEventStatus=0x0012f358)  Line 5981 + 0x1b bytes	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x07d101d0, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f594, int aCaptured=0)  Line 1665	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f594, nsEventStatus * aStatus=0x0012f480)  Line 1618 + 0x22 bytes	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f594)  Line 174	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f594, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1102 + 0xc bytes	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f594)  Line 1123	C++
 	gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=301, unsigned int wParam=0, long lParam=3014724)  Line 6033 + 0x1a bytes	C++
 	gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=301, unsigned int wParam=0, long lParam=3014724)  Line 6216	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=514, unsigned int wParam=0, long lParam=3014724, long * aRetValue=0x0012fa48)  Line 4548 + 0x20 bytes	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00310560, unsigned int msg=514, unsigned int wParam=0, long lParam=3014724)  Line 1291 + 0x1d bytes	C++
 	user32.dll!77d48734() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
 	user32.dll!77d48816() 	
 	user32.dll!77d489cd() 	
 	user32.dll!77d49402() 	
 	user32.dll!77d48a10() 	
 	gkwidget.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=1)  Line 149	C++
 	gkwidget.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=1)  Line 136 + 0x11 bytes	C++
 	gkwidget.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x00b3a0c8, int mayWait=1, unsigned int recursionDepth=0)  Line 231 + 0xf bytes	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fc34)  Line 472	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00b3a0c8, int mayWait=1)  Line 225 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 153 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 171 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00b38150, const nsXREAppData * aAppData=0x004036b0)  Line 2387 + 0x25 bytes	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00b38150)  Line 61 + 0x13 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816d4f() 	
 	kernel32.dll!7c8399f3()

Comment 1

[(not reading, please use [e-mail] instead)]

fwiw, I see lots of talkbacks with pref_DoCallback.

unfortunately, my crasher isn't 100% reproducable.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

I can't reproduce with current trunk build.
Can you reproduce with this testcase (you need to download it locally because of the use of enhanced privileges)?

Comment 3

[(not reading, please use [e-mail] instead)]

martijn, thanks for that test case.  I can use it to reproduce the crash.

try this:

1)  open two browser windows
2)  in window #1, load martijn's test case (after saving it to disk)
3)  close window #2

a few seconds later, boom. 

This is reproducable.

the console error messages are particularly interesting / scary.  I'll attach them.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: the console messages

Comment 5

[(not reading, please use [e-mail] instead)]

lots of these before the crash:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' pr
operty!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file c:/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativesco
pe.cpp, line 587

Comment 6

[timeless]

usually the pref callback crash is because someone is trying to munge a pref in response to a pref change or something similarly evil.

the warning you're getting is because a dom hosted js object (tabbrowser) has installed hooks into objects which outlive the window it lives in. so the window that hosted tabbrowser is gone, but the objects are still around trying to do work. the js in question needs to be fixed.

Comment 7

[(not reading, please use [e-mail] instead)]

the comment from timeless sounds like it might be related to session restore?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Needs investigation

Comment 9

[Nickolay_Ponomarev]

Fun. The browser.tabs.closeButtons observer is registered with aHoldWeak == true:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.206#2651
(and not unregistered on binding destruction)

The code in NotifyObserver in nsPrefBranch.cpp removes the weak reference to a dead observer:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpref/src/nsPrefBranch.cpp&rev=1.69&mark=740-741#736

The problem is that NotifyObserver is called while enumerating observers in pref_DoCallback, and removing the current item in the gCallbacks list invalidates its |next| property.

This has an obvious fix, but the assertion still happens even with the fix. Interestingly, it only happens when not running in the debugger, and the debugger is pretty confused about the stack when we hit the assertion. Still investigating...

Comment 10

[Nickolay_Ponomarev]

Attachment: patch

This fixes the crash.

The assertion still fires. For some reason, the observer object in the closed window is not, in fact, destroyed when the preference is changed (although the binding's destructor has already run). I don't know enough about how window teardown happens to figure out why this is so.

The binding can easily be patched to remove the observer in its destructor, but I'd like to know why it doesn't work now first.

Comment 11

[Nickolay_Ponomarev]

Comment on attachment 242159 [details] [diff] [review]
patch

Duh. Nothing stops the observer itself from removing the next items from the list, so this may still crash.

Comment 12

[Nickolay_Ponomarev]

So, I'm not sure what is the proper fix to this, given that
1) it is possible to add/remove random observers during the notification stage.
2) it seems possible to re-enter pref_DoCallback. So even if we forbid making any changes to the list, except for removing current item, it doesn't solve the problem.
3) gCallbacks list is long (> 300 items right after firefox startup), so I'd rather not copy it each time pref changed notification happens.
4) adding fields to CallbackNode is relatively expensive (I had an idea to add a |PRBool dead| field and postpone the actual removals until all notifications are done).

Maybe keep a list of notified observers in pref_DoCallback and start over (using the list we've created to avoid sending duplicate notifications) when we detect that gCallbacks list has changed. If we do that, the cost of normal notifications won't be much higher, but it will be higher for notifications that caused changes in the observers list.

Comment 13

[Nickolay_Ponomarev]

Or 5) disallow any changes to the list while in pref_DoCallback. That may break people, which is not good for people relying on frozen functionality.
(Dead references can be indicated via special rv and deleted at the end of pref_DoCallback.)

Comment 14

[Nickolay_Ponomarev]

Attachment: patch, use a 'dead' property during the notification stage

OK, I think the right thing to do is to mark nodes for deletion if someone attempts to delete them from a pref observer. This does cost us an extra field in the callbacks list, but it doesn't make the code much more complex and should have backwards-compatible behavior.

Comment 15

[Nickolay_Ponomarev]

Comment on attachment 242172 [details] [diff] [review]
patch, use a 'dead' property during the notification stage

>+        gShouldCleanupDeadNodes = PR_TRUE;

I mean gShouldCleanupDeadNodes = PR_FALSE. I shouldn't write patches while not paying attention.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I'm pretty swamped right now, and this is not exactly my area of expertise...  Might be worth looking for a different sr.  I probably won't be able to get to this patch for weeks.

Comment 17

[Nickolay_Ponomarev]

Attachment: patch

Comment 18

[Jesse Ruderman]

Another possible fix is to factor out the "array with mutation-safe iterator" class from bug 340733 and use it here.  Looks like this code uses a linked list while the code in bug 340733 used an array; I don't know which data structure is faster or easier to get right when trying to do mutation-safe iteration.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Another possible fix is to factor out the "array with mutation-safe iterator"

Not so easy; this code is in a different module.  That code _has_ been refactored into nsTObserverArray, fwiw.

Comment 20

[Jesse Ruderman]

Maybe nsTObserverArray should be moved to xpcom along with nsIObserver and nsTArray?

Comment 21

[Nickolay_Ponomarev]

Filed bug 356599 for the assertion.

Comment 22

[Nickolay_Ponomarev]

Attachment: testcase

A testcase that doesn't depend on a bug in Firefox chrome. This one removes an observer in observe(), which is not strictly the same as the original bug, but is pretty close.

Comment 23

[timeless]

> +   * It is possible to change preferences during the notification.

"the notification" isn't what you want to write (but I'm not sure what is...)

Comment 24

[Nickolay_Ponomarev]

Attachment: patch

Updated to the tip and included the test in the patch (via xpcshell test harness). I would appreciate a quick comment if you want me to take a different approach (like using an array class like discussed above) here, since that may take a while given the amount of free time I have.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We've seen intermittent crashes here and it would awesome to get this fixed.

Bonsai says that darin is the only one who has really reviewed this code since 2003, so perhaps he could review this?

Comment 26

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 263663 [details] [diff] [review]
patch

Benjamin, can you review this?

Comment 28

[Benjamin Smedberg]

Comment on attachment 263663 [details] [diff] [review]
patch

Boy is this ugly. But it at least works, and I can't think of a simpler way to do it at the moment. I'd like Darin's opinion of this, since he used to own prefservice.

Comment 29

[timeless]

Comment on attachment 263663 [details] [diff] [review]
patch

it should be possible to create an nsVoidArray of things to be removed, and an nsVoidArray of things to be added. while you're nesting callbacks, you don't perform any removals/additions, merely queue them. that avoids adding an extra field to callbacknode.

I'm not saying you have to do that, but you could. i'm not a fan of adding fields to CallbackNode

Comment 31

[Darin Fisher]

Comment on attachment 263663 [details] [diff] [review]
patch

OK, but do you really need another field in the struct?  It seems like you could also just clear the function pointer and use that as a "is dead" sentinel.  no?

Comment 32

[Nickolay_Ponomarev]

Attachment: patch, as darin suggested

Comment 33

[Nickolay_Ponomarev]

Attachment: patch, the right one

Comment 34

[timeless]

Comment on attachment 272359 [details] [diff] [review]
patch, the right one

>Index: modules/libpref/public/nsIPrefBranch2.idl
>@@ -88,16 +88,26 @@ interface nsIPrefBranch2 : nsIPrefBranch
>+   * @note
>+   * The list of registered observers may be changed during the dispatch of
>+   * nsPref:changed notification. However, the observers are not guaranteed
>+   * to be notified in any particular order, so you can't be sure if the

... sure whether the ...

>+   * added/removed observer will be called during the notification when it
>+   * was added/removed.

is added/removed.

>+   * @note
>+   * It is possible to change preferences during the notification.

@note
This is not safe to change observers during this callback in Gecko releases before 1.9. If you want a safe way to remove a pref observer, please use an nsITimer.

> static nsresult pref_DoCallback(const char* changed_pref)
>+    // No nodes must be deleted from the list while gCallbacksInProgress

Nodes must not be deleted [...] while gCallbacksInProgress

(i wouldn't bother with this ellipsized text)

>+    // is PR_TRUE. Nodes that need to be deleted are marked by nulling

... are marked for deletion by ...

>+    // out the |func| pointer. They are taken care of at the end of this

. We release them at the end of this function if we haven't reentered.

>+    // function.

Comment 35

[Marc Bejarano]

OS, Hardware -> All based on reports from Bug 361921

Comment 36

[Nickolay_Ponomarev]

Attachment: patch with timeless' nits addressed

Comment 37

[Mark Macdonald]

Anyone have a status on this? It's been almost a month since the last update.

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

(Waking from a long silence)
This should certainly work and solves the issue (crash).  It would be nice to remove the need for this sort of kludge in the future, but that should not block getting this in.  My suggestion would be to open a new bug on the design issue here to capture that this needs attention at some point, once this fix is checked in.

Let's get this in...

Comment 39

[Benjamin Smedberg]

Comment on attachment 273692 [details] [diff] [review]
patch with timeless' nits addressed

I'm going to mark sr+ on this based on my and darin's original review... let's get this landed

Comment 40

[Reed Loden [:reed]]

Checking in modules/libpref/Makefile.in;
/cvsroot/mozilla/modules/libpref/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in modules/libpref/public/nsIPrefBranch2.idl;
/cvsroot/mozilla/modules/libpref/public/nsIPrefBranch2.idl,v  <--  nsIPrefBranch2.idl
new revision: 1.5; previous revision: 1.4
done
Checking in modules/libpref/src/prefapi.cpp;
/cvsroot/mozilla/modules/libpref/src/prefapi.cpp,v  <--  prefapi.cpp
new revision: 3.145; previous revision: 3.144
done
RCS file: /cvsroot/mozilla/modules/libpref/test/Makefile.in,v
done
Checking in modules/libpref/test/Makefile.in;
/cvsroot/mozilla/modules/libpref/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libpref/test/unit/test_bug345529.js,v
done
Checking in modules/libpref/test/unit/test_bug345529.js;
/cvsroot/mozilla/modules/libpref/test/unit/test_bug345529.js,v  <--  test_bug345529.js
initial revision: 1.1
done

Comment 41

[Nickolay_Ponomarev]

Thanks benjamin and reed! Anyone thinks we need a bug on replacing this code with nsTObserverArray?

Comment 42

[Nickolay_Ponomarev]

BTW I can't help with getting this into 1.8, since I can't build that anywhere. So if anyone needs it on 1.8, feel free to adapt the patch.

Comment 43

[Philip Chee]

Now that 3.0b2 is out does anyone have some spare cycles to backport this to 1.8.1.x?

Comment 44

[timeless]

afaict this patch stands alone and applies perfectly to the branch, someone should test it and just ask for approval on the patch.

As for this comment:
+   * It is not safe to change observers during this callback in Gecko 
+   * releases before 1.9. If you want a safe way to remove a pref observer,
+   * please use an nsITimer.

In theory the comment could be changed to reflect whichever version of 1.8.x gets the fix, but I'd rather leave the comment as is. Because it's true for most releases before 1.9 (including many alphas).

Comment 45

[Daniel Veditz [:dveditz]]

Attachment: 1.8 branch version (minor edit)

Minor merge to generate 1.8 version.

Comment 46

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

Comment on attachment 299829 [details] [diff] [review]
1.8 branch version (minor edit)

We're not going to take this until the next release.

Comment 47

[Daniel Veditz [:dveditz]]

Comment on attachment 299829 [details] [diff] [review]
1.8 branch version (minor edit)

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 48

[Daniel Veditz [:dveditz]]

Fix checked into 1.8 branch

Comment 49

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

(In reply to comment #48)
> Fix checked into 1.8 branch
> 

Does this actually reproduce in the 1.8 branch? I've tried both of the attached testcases (comment 3 and comment 22) and neither crashes in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/2008020121 Firefox/2.0.0.12, which is from before this fix was checked in.

Comment 50

[Philip Chee]

> Does this actually reproduce in the 1.8 branch? 

Bug 361921 which is duplicated to this is a branch crash bug. You'll need to test that on OSX.

Comment 51

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

Philip, there aren't really any repro steps for Bug 361921 in that bug beyond allowing flash on a site in flashblock. I haven't been able to get a crash and I'm running it in 2.0.0.12 on OS X 10.5.2. So, I haven't been able to verify this fix yet since I can't get the crash to occur.

Comment 52

[Alexander Sack]

Comment on attachment 299829 [details] [diff] [review]
1.8 branch version (minor edit)

taking this for distro patches, caillon, please verify and approve.

Comment 53

[Marc Bejarano]

i can confirm that i no longer crash as i always did with bug 361921 using fx 2.0.0.13