Bug 34153

Summary:

Crash in WebCore while processing the fast scrolling code path.

Product:

WebKit

Reporter:

Ananta Iyengar <ananta>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, darin, dglazkov, eric, evan, hausmann, hyatt, jamesr, levin

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows XP

Attachments:

Description	Flags
Unregister the RenderObject in corner cases	simon.fraser: review-
Revert 53797 until we land	simon.fraser: review+

Ananta Iyengar

Reported 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

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
View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2010-01-25 21:29:17 PST

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

Ananta Iyengar

Comment 2 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.

Benjamin Poulain

Comment 3 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.

Benjamin Poulain

Comment 4 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.

James Robinson

Comment 5 2010-01-26 17:04:23 PST

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

Evan Martin

Comment 6 2010-01-26 17:08:50 PST

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

Simon Fraser (smfr)

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

Jeremy Orlow

Comment 8 2010-01-26 17:20:01 PST

Created attachment 47472 [details] Revert 53797 until we land

Benjamin Poulain

Comment 9 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.

Benjamin Poulain

Comment 10 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.

Note You need to log in before you can comment on or make changes to this bug.