Bug 34153 – Crash in WebCore while processing the fast scrolling code path.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ananta Iyengar]

This crash appears to be a regression of this changelist http://trac.webkit.org/changeset/53693 for bug
https://bugs.webkit.org/show_bug.cgi?id=33150. The crash has been consistently occurring in Chromium reliability
test runs.

The callstack can be found here.

http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Reliability/builds/7979/steps/reliability:%20partial%20result%20of%20current%20build/logs/stdio

[Eric Seidel (no email)]

Happy to revert the change if we can confirm it's that revision.

[Ananta Iyengar]

I looked at the reliability test runs from Friday. The crashes started occurring from the time we bumped up the DEPS to pull in the corresponding webkit revision. The only change in the revision of webkit which could cause this appears to be this change. We can try reverting it and see if this is indeed the case.

[Benjamin Poulain]

This would be my fault. I am getting Chronium to see what's wrong, I hope I can compile it quickly on Linux.

[Benjamin Poulain]

Created attachment 47403 [details]
Unregister the RenderObject in corner cases

This patch should fix the problem for the websites listed.

The problem is deriving from the change of https://bugs.webkit.org/show_bug.cgi?id=33150
RenderWidget::destroy() is special and do not call ::destroy() on the parent. The code for cleaning the state must be copied in that class (as said in the comments of RenderWidget::destroy()).

It's great you have those tests.

[James Robinson]

Could http://trac.webkit.org/changeset/53797 be reverted until this crash is resolved?

[Evan Martin]

Typo in changelog:
+        The code to unregister the object with fixed positionning must

[Simon Fraser (smfr)]

Comment on attachment 47403 [details]
Unregister the RenderObject in corner cases

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +        The code to unregister the object with fixed positionning must
> +        be copied to RenderWidget.

"positionning"

> +        Unregistering must also be done when a render widget get a 
> +        transformation.

Not just a RenderWidget, but any RenderObject. "gets", not "get".

> diff --git a/WebCore/rendering/RenderWidget.cpp b/WebCore/rendering/RenderWidget.cpp

>      animation()->cancelAnimations(this);
>  
> -    if (RenderView* v = view())
> +    if (RenderView* v = view()) {
>          v->removeWidget(this);
> +        FrameView* frameView = view()->frameView();
> +        if (frameView)
> +            frameView->unregisterFixedPositionedObject(this);
> +    }

Why doesn't this have the "m_style->position() == FixedPosition" check that RenderObject does? Is one of them wrong?

This also needs a layout test. r- for that.

[Jeremy Orlow]

Created attachment 47472 [details]
Revert 53797 until we land

[Benjamin Poulain]

(In reply to comment #7)
> >      animation()->cancelAnimations(this);
> >  
> > -    if (RenderView* v = view())
> > +    if (RenderView* v = view()) {
> >          v->removeWidget(this);
> > +        FrameView* frameView = view()->frameView();
> > +        if (frameView)
> > +            frameView->unregisterFixedPositionedObject(this);
> > +    }
> 
> Why doesn't this have the "m_style->position() == FixedPosition" check that
> RenderObject does? Is one of them wrong?

Yep, in the hurry I forgot the "m_style->position() == FixedPosition" for this one.

(In reply to comment #8)
> Created an attachment (id=47472) [details]
> Revert 53797 until we land

Ok.

[Benjamin Poulain]

Closing the task:
The original patch has been reverted. The upgraded version is on https://bugs.webkit.org/show_bug.cgi?id=33150 for review.