Bug 55557 - Override paintScrollCorner() for FramelessScrollView to do nothing (fixes a crash)
Summary: Override paintScrollCorner() for FramelessScrollView to do nothing (fixes a c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ilya Sherman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-01 23:49 PST by Ilya Sherman
Modified: 2011-03-04 08:07 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.20 KB, patch)
2011-03-01 23:51 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2011-03-02 02:06 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (5.12 KB, patch)
2011-03-02 20:21 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.38 KB, patch)
2011-03-03 00:44 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2011-03-01 23:49:50 PST
Override paintScrollCorner() for FramelessScrollView to do nothing (fixes a crash)
Comment 1 Ilya Sherman 2011-03-01 23:51:56 PST
Created attachment 84377 [details]
Patch
Comment 2 Kent Tamura 2011-03-02 01:29:55 PST
Comment on attachment 84377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84377&action=review

> Source/WebCore/platform/chromium/FramelessScrollView.cpp:79
> +void FramelessScrollView::paintScrollCorner(GraphicsContext*, const IntRect& cornerRect)

should remove the unused argument name.
Comment 3 Kent Tamura 2011-03-02 01:31:39 PST
Probably fishd or eric is familiar with this code.

I don't understand why this change fixes the crash.  We need to add a reason to the patch.
Comment 4 Eric Seidel (no email) 2011-03-02 01:35:39 PST
Comment on attachment 84377 [details]
Patch

What does the crash look like?  What's the test case to repro the crash?

I doubt this is the right fix w/o that info.
Comment 5 Ilya Sherman 2011-03-02 02:06:13 PST
Created attachment 84383 [details]
Patch
Comment 6 Ilya Sherman 2011-03-02 02:07:25 PST
(In reply to comment #4)
> (From update of attachment 84377 [details])
> What does the crash look like?  What's the test case to repro the crash?

Take a look at http://crbug.com/73772 -- esp crbug.com/73772#c23
Comment 7 James Robinson 2011-03-02 16:27:01 PST
Comment on attachment 84383 [details]
Patch

Why isn't this testable?
Comment 8 James Robinson 2011-03-02 17:23:51 PST
Comment on attachment 84383 [details]
Patch

I assume the bad cast you are referring to is this one: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollbarThemeComposite.cpp#L309 ?

What does the comment in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp#L140 mean, then?

Something somewhere here is inconsistent - not sure if it's the code or the comments.
Comment 9 Ilya Sherman 2011-03-02 17:49:06 PST
(In reply to comment #8)
> (From update of attachment 84383 [details])
> I assume the bad cast you are referring to is this one: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollbarThemeComposite.cpp#L309 ?

Yep, that's the one.

> What does the comment in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp#L140 mean, then?

On Mac, we never reach ScrollbarThemeChromium... probably because we want to use native scroll widgets?  Perhaps it would be more appropriate to move the code from ScrollbarThemeChromium.cpp to FramelessScrollView.cpp?
Comment 10 Ilya Sherman 2011-03-02 18:04:28 PST
(In reply to comment #7)
> (From update of attachment 84383 [details])
> Why isn't this testable?

On non-Mac platforms, trying to bring up the select popup crashes DRT.  On Mac, we have a custom <select> popup, and there's currently no way to bring up the FramelessScrollView popup that autofill uses.  Also, if we could bring that up, it would probably crash DRT for the same reason as on other platforms -- which I think is something along the lines of DRT isn't prepared to show this new window object.
Comment 11 James Robinson 2011-03-02 18:11:01 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 84383 [details] [details])
> > I assume the bad cast you are referring to is this one: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollbarThemeComposite.cpp#L309 ?
> 
> Yep, that's the one.
> 
> > What does the comment in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp#L140 mean, then?
> 
> On Mac, we never reach ScrollbarThemeChromium... probably because we want to use native scroll widgets?  Perhaps it would be more appropriate to move the code from ScrollbarThemeChromium.cpp to FramelessScrollView.cpp?

Maybe - we definitely want to only have one solution to this problem, not two.
Comment 12 Ilya Sherman 2011-03-02 20:21:31 PST
Created attachment 84516 [details]
Patch
Comment 13 James Robinson 2011-03-02 23:43:41 PST
Comment on attachment 84516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84516&action=review

> Source/WebCore/platform/chromium/FramelessScrollView.cpp:83
> +    ScrollbarTheme().paintScrollCorner(this, context, cornerRect);

This looks weird.  We just make a ScrollbarTheme out of nowhere and use it?

I'm not very familiar with the relationship between a Scrollbar, ScrollView, and ScrollbarTheme but this is definitely a code smell.
Comment 14 Ilya Sherman 2011-03-03 00:35:30 PST
Comment on attachment 84516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84516&action=review

>> Source/WebCore/platform/chromium/FramelessScrollView.cpp:83
>> +    ScrollbarTheme().paintScrollCorner(this, context, cornerRect);
> 
> This looks weird.  We just make a ScrollbarTheme out of nowhere and use it?
> 
> I'm not very familiar with the relationship between a Scrollbar, ScrollView, and ScrollbarTheme but this is definitely a code smell.

So, ScrollbarTheme::paintScrollCorner() provides a default implementation of the method, and is essentially a static function.  Two obvious alternatives to writing the code this way are:

1) Copy the default implementation into FramelessScrollView::paintsScrollCorner.
2) Add a static function to ScrollbarTheme that implements the default action, and call that.

I'm not a fan of code duplication, so option (1) is out.  I'm not sure that option (2) is great either, but I guess I'll switch to that for now.  Do you have a moar awesomer suggestion?
Comment 15 Ilya Sherman 2011-03-03 00:44:59 PST
Created attachment 84532 [details]
Patch
Comment 16 James Robinson 2011-03-03 13:29:06 PST
Comment on attachment 84532 [details]
Patch

I don't have a moar awesome suggestion, but this seems at least a bit cleaner than the previous iterations.  R=me, especially since this is a top crasher, but if anyone has an even better idea I'm open to it!
Comment 17 WebKit Commit Bot 2011-03-04 08:04:48 PST
The commit-queue encountered the following flaky tests while processing attachment 84532 [details]:

security/block-test.html bug 55741 (authors: beidson@apple.com, mrowe@apple.com, and sam@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2011-03-04 08:07:32 PST
Comment on attachment 84532 [details]
Patch

Clearing flags on attachment: 84532

Committed r80353: <http://trac.webkit.org/changeset/80353>
Comment 19 WebKit Commit Bot 2011-03-04 08:07:38 PST
All reviewed patches have been landed.  Closing bug.