Bug 153695 - REGRESSION(r195661): [GTK] Scrollbar tests crashing after overlay scrollbar groundwork
Summary: REGRESSION(r195661): [GTK] Scrollbar tests crashing after overlay scrollbar g...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 153936 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-29 20:17 PST by Michael Catanzaro
Modified: 2016-03-05 05:24 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2016-02-06 03:20 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-01-29 20:17:51 PST
fast/repaint/fixed-move-after-scroll.html has been crashing flakily beginning with r195672-r195673. These two commits are obviously not the cause of the crash, but they come shortly after several commits implementing the groundwork for overlay scrollbars landed.
Comment 1 Michael Catanzaro 2016-01-29 20:31:07 PST
And fast/scrolling/scroll-position-on-reload-rtl.html has been fairly reliably since r195660-r195661. r195660 looks like a more likely culprit so I'll pick that for the bug title.
Comment 2 Michael Catanzaro 2016-01-29 20:31:26 PST
(In reply to comment #1)
> And fast/scrolling/scroll-position-on-reload-rtl.html has been fairly
> reliably since r195660-r195661.

fairly reliably crashing
Comment 3 Michael Catanzaro 2016-02-05 20:39:27 PST
Also scrollbars/scrollbar-initial-position.html (flaky since r195660-r195661).
Comment 4 Carlos Garcia Campos 2016-02-05 23:38:21 PST
When reporting this kind of failures it's very useful to include links to the bots results, especially if they are flaky. That way I know if it's an assert only happening in debug or if it also fails in release, and I can focus on fixing the bug instead of figuring out how to make it crash.
Comment 5 Carlos Garcia Campos 2016-02-06 03:16:32 PST
*** Bug 153936 has been marked as a duplicate of this bug. ***
Comment 6 Carlos Garcia Campos 2016-02-06 03:20:02 PST
Created attachment 270792 [details]
Patch
Comment 7 Michael Catanzaro 2016-02-06 07:37:38 PST
(In reply to comment #4)
> When reporting this kind of failures it's very useful to include links to
> the bots results, especially if they are flaky. That way I know if it's an
> assert only happening in debug or if it also fails in release, and I can
> focus on fixing the bug instead of figuring out how to make it crash.

OK
Comment 8 Michael Catanzaro 2016-02-06 07:43:18 PST
Comment on attachment 270792 [details]
Patch

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

Also update the test expectations?

> Source/WebCore/platform/ScrollAnimation.h:38
> +    virtual ~ScrollAnimation() { };

Ugh, real shame this was not caught by -Wdelete-non-virtual-dtor.
Comment 9 Carlos Garcia Campos 2016-02-07 01:43:51 PST
(In reply to comment #8)
> Comment on attachment 270792 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270792&action=review
> 
> Also update the test expectations?

Ah, did you also update the test expectations? It's also very useful to add a comment to the bug when you do so, saying, expectations update in r123456.
Comment 10 Carlos Garcia Campos 2016-02-07 02:09:42 PST
Committed r196238: <http://trac.webkit.org/changeset/196238>
Comment 11 Michael Catanzaro 2016-02-07 07:18:51 PST
(In reply to comment #9)
> Ah, did you also update the test expectations? It's also very useful to add
> a comment to the bug when you do so, saying, expectations update in r123456.

OK, I'll try to start doing this. (I always update expectations when filing bugs against layout tests.)
Comment 12 Darin Adler 2016-03-04 15:32:16 PST
Iā€™d like to understand where this was being deleted in a polymorphic way, but with something other than delete. If it was being deleted with delete, then -Wdelete-non-virtual-dtor should have warned us about this mistake.
Comment 13 Michael Catanzaro 2016-03-04 17:02:18 PST
I think through std::unique_ptr. A quick test:

#include <memory>

class A
{
public:
  virtual void something() { }
};

class B : public A
{
};

This code triggers -Wdelete-non-virtual-dtor in both GCC and Clang:

int main()
{
  A* a = new B;
  delete a;
}

But this code triggers only -Wnon-virtual-dtor:

int main()
{
  auto a = std::make_unique<B>();
}
Comment 14 Michael Catanzaro 2016-03-04 17:04:46 PST
Just found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58876

"""This is because warnings are suppressed by default in system headers, and the undefined delete operation occurs in a system header.  You get the warning if you use -Wsystem-headers"""

(Of course we don't want to do that though, we'd be spammed by tons of warnings we can't control.)
Comment 15 Michael Catanzaro 2016-03-05 05:24:37 PST
(In reply to comment #13)
> But this code triggers only -Wnon-virtual-dtor:
> 
> int main()
> {
>   auto a = std::make_unique<B>();
> }

Well that's a bad example because that's not polymorphic, the right example is:

int main()
{
  auto a = std::unique_ptr<A>(new B);
}

(It still triggers only -Wnon-virtual-dtor, so my point is sound.)