Override paintScrollCorner() for FramelessScrollView to do nothing (fixes a crash)

• Component: New Bugs
• Status: RESOLVED FIXED

[Ilya Sherman]

Override paintScrollCorner() for FramelessScrollView to do nothing (fixes a crash)

[Ilya Sherman]

Created attachment 84377 [details]
Patch

[Kent Tamura]

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.

[Kent Tamura]

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.

[Eric Seidel (no email)]

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.

[Ilya Sherman]

Created attachment 84383 [details]
Patch

[Ilya Sherman]

(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

[James Robinson]

Comment on attachment 84383 [details]
Patch

Why isn't this testable?

[James Robinson]

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.

[Ilya Sherman]

(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?

[Ilya Sherman]

(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.

[James Robinson]

(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.

[Ilya Sherman]

Created attachment 84516 [details]
Patch

[James Robinson]

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.

[Ilya Sherman]

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?

[Ilya Sherman]

Created attachment 84532 [details]
Patch

[James Robinson]

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!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 84532 [details]:

security/block-test.html bug 55741 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 84532 [details]
Patch

Clearing flags on attachment: 84532

Committed r80353: <http://trac.webkit.org/changeset/80353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.