Bug 34153 - Crash in WebCore while processing the fast scrolling code path.
Summary: Crash in WebCore while processing the fast scrolling code path.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-25 20:37 PST by Ananta Iyengar
Modified: 2010-01-28 08:52 PST (History)
9 users (show)

See Also:


Attachments
Unregister the RenderObject in corner cases (3.35 KB, patch)
2010-01-26 04:49 PST, Benjamin Poulain
simon.fraser: review-
Details | Formatted Diff | Diff
Revert 53797 until we land (11.07 KB, patch)
2010-01-26 17:20 PST, Jeremy Orlow
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ananta Iyengar 2010-01-25 20:37:08 PST
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
Comment 1 Eric Seidel (no email) 2010-01-25 21:29:17 PST
Happy to revert the change if we can confirm it's that revision.
Comment 2 Ananta Iyengar 2010-01-25 23:42:13 PST
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.
Comment 3 Benjamin Poulain 2010-01-26 01:57:53 PST
This would be my fault. I am getting Chronium to see what's wrong, I hope I can compile it quickly on Linux.
Comment 4 Benjamin Poulain 2010-01-26 04:49:53 PST
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.
Comment 5 James Robinson 2010-01-26 17:04:23 PST
Could http://trac.webkit.org/changeset/53797 be reverted until this crash is resolved?
Comment 6 Evan Martin 2010-01-26 17:08:50 PST
Typo in changelog:
+        The code to unregister the object with fixed positionning must
Comment 7 Simon Fraser (smfr) 2010-01-26 17:17:58 PST
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.
Comment 8 Jeremy Orlow 2010-01-26 17:20:01 PST
Created attachment 47472 [details]
Revert 53797 until we land
Comment 9 Benjamin Poulain 2010-01-27 00:04:36 PST
(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.
Comment 10 Benjamin Poulain 2010-01-28 08:52:48 PST
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.