381153 - crash [@ nsSliderFrame::SetCurrentPositionInternal]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch, using weakframes

See url for a testcase.

I'm not happy to use weakframes again, but can't see any other way to
fix this (easily).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+         NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
          return NS_OK;

Why?

        SetCurrentPosition(scrollbar, pospx, PR_FALSE);
+       NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);

Can you avoid the need here by removing mRedrawImmediate and just passing it as a parameter through SetCurrentPosition/SetCurrentPositionInternal/CurrentPositionChanged?

+    NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
 
     DragThumb(PR_TRUE);

Can we avoid the need here by moving DragThumb() and the other code below to above the SetCurrentPosition call? Maybe in general MouseDown should just end with a SetCurrentPosition call, so every path just sets pospx?

+      nsWeakFrame weakFrame(this);
       mediator->PositionChanged(scrollbarFrame, GetCurrentPosition(scrollbar), newpos);
       // 'mediator' might be dangling now...
+      ENSURE_TRUE(weakFrame.IsAlive());
       UpdateAttribute(scrollbar, newpos, PR_FALSE, aIsSmooth);
+      ENSURE_TRUE(weakFrame.IsAlive());
       CurrentPositionChanged(PresContext());

UpdateAttribute is static so if we hold a strong ref to the scrollbar element, this is safe.

For CurrentPositionChanged, we could re-get the frame and call CurrentPositionChanged on it, right?

+    nsWeakFrame weakFrame(this);
     SetCurrentPosition(scrollbar, pospx, PR_FALSE);
+    NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
   }
 
   DragThumb(PR_TRUE);

Same sort of thing as above...

+  nsWeakFrame weakFrame(this);
   PageUpDown(change);
+  NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
   
   nsRepeatService::GetInstance()->Start(mMediator);

Just start the repeatservice before calling PageUpDown...

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 265246 [details] [diff] [review]
possible patch, using weakframes

Will update the patch.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> UpdateAttribute is static so if we hold a strong ref to the scrollbar element,
> this is safe.
> 

It may still kill the frame.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Ah, sorry, you meant something else

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

This has only one weakFrame, in ::HandleEvent to check that whether
nsFrame::HandleEvent can be called.
The testcase doesn't trigger that weakFrame.IsAlive() warning.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 265927 [details] [diff] [review]
v2

great!

+  nscoord pospx;

set it to zero to avoid compiler warnings

Comment 7

[Daniel Veditz [:dveditz]]

Crashes on 1.8 branch also

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Will do 1.8 patch, maybe just using weakframes to minimize regression
risk. 

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

using more weakframes and changing some nsCOMPtrs to raw refs.
Should be safe for branches.
It seems that GetScrollbarMediator returns an nsIFrame object which 
isn't refcounted, so nsCOMPtr should be used.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Would it make more sense to stick close to the trunk? We'll get better test coverage in both directions that way.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

The trunk patch will have baking time, but I really wouldn't want to 
change any functionality in branches. We have had quite some regressions 
in 1.8.1.x. The branch patch is ugly-but-safe, trunk patch not-so-ugly-but-may-cause-regressions (though, regressions are very unlikely).

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Roc, do you want a new, more trunk-like 1.8 patch?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think so, if that's OK

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 for 1.8

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 267949 [details] [diff] [review]
v2 for 1.8

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 16

[Carsten Book [:Tomcat]]

Verified fixed on Trunk using  Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a7pre) Gecko/200707110505 Minefield/3.0a7pre and the testcase from https://bugzilla.mozilla.org/attachment.cgi?id=265242 - no crash on Testcase  

Also verified fixed 1.8.1.5 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/2007071103 BonEcho/2.0.0.5pre and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/2007071103 BonEcho/2.0.0.5pre on Fedora F7 - no crash on testcase - adding verified keyword

Comment 17

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

This doesn't crash Thunderbird 1.5.0.13 (2007080918) but I cannot get it to crash 1.5.0.12 either.

Comment 18

[Carsten Book [:Tomcat]]

verified fixed 1.8.0.13 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre

No crash on testcase - adding verified keyword.