Bug 153695 – REGRESSION(r195661): [GTK] Scrollbar tests crashing after overlay scrollbar groundwork

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(In reply to comment #1)
> And fast/scrolling/scroll-position-on-reload-rtl.html has been fairly
> reliably since r195660-r195661.

fairly reliably crashing

[Michael Catanzaro]

Also scrollbars/scrollbar-initial-position.html (flaky since r195660-r195661).

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

*** Bug 153936 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 270792 [details]
Patch

[Michael Catanzaro]

(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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r196238: <http://trac.webkit.org/changeset/196238>

[Michael Catanzaro]

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

[Darin Adler]

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.

[Michael Catanzaro]

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>();
}

[Michael Catanzaro]

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

[Michael Catanzaro]

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