Bug 89785 - REGRESSION(r116446): Crash in RenderBoxModelObject::adjustedPositionRelativeToOffsetParent
Summary: REGRESSION(r116446): Crash in RenderBoxModelObject::adjustedPositionRelativeT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-22 14:37 PDT by Julien Chaffraix
Modified: 2012-06-22 16:30 PDT (History)
5 users (show)

See Also:


Attachments
Proposed fix. NULL-check parent. (4.91 KB, patch)
2012-06-22 15:20 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (4.52 KB, patch)
2012-06-22 16:02 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-06-22 14:37:08 PDT
Sample backtrace:

0x66f497aa	 [chrome.dll]	 - renderboxmodelobject.cpp:526]	WebCore::RenderBoxModelObject::adjustedPositionRelativeToOffsetParent(WebCore::FractionalLayoutPoint const &)
0x66f4965c	 [chrome.dll]	 - renderbox.cpp:3839]	WebCore::RenderBox::offsetLeft()
0x67554103	 [chrome.dll]	 - rendervideo.cpp:317]	WebCore::RenderVideo::offsetLeft()
0x672321ef	 [chrome.dll]	 - element.cpp:372]	WebCore::Element::offsetLeft()
0x6768aad9	 [chrome.dll]	 - v8element.cpp:74]	WebCore::ElementV8Internal::offsetLeftAttrGetter
0x66df9893	 [chrome.dll]	 - objects.cc:203]	v8::internal::JSObject::GetPropertyWithCallback(v8::internal::Object *,v8::internal::Object *,v8::internal::String *)
0x66c3e9d1	 [chrome.dll]	 - objects.cc:633]	v8::internal::Object::GetProperty(v8::internal::Object *,v8::internal::LookupResult *,v8::internal::String *,PropertyAttributes *)
0x66dba41f	 [chrome.dll]	 - ic.cc:933]	v8::internal::LoadIC::Load(v8::internal::InlineCacheState,v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::String>)

I haven't been able to find out how users are hitting the bug. It's also very infrequent which makes it difficult to find a good reproduction.

The bug is fairly obvious once you look at the failing line:

referencePoint.move(parent()->offsetForColumns(referencePoint));

We need to NULL-check parent() as is done in offsetParent().
Comment 1 Julien Chaffraix 2012-06-22 15:20:56 PDT
Created attachment 149119 [details]
Proposed fix. NULL-check parent.
Comment 2 Abhishek Arya 2012-06-22 15:32:41 PDT
Comment on attachment 149119 [details]
Proposed fix. NULL-check parent.

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

> LayoutTests/fullscreen/full-screen-crash-offsetLeft.html:16
> +        if (window.testRunner) {

these four lines are already provided by full-screen-test.js. no need of them here.

> LayoutTests/fullscreen/full-screen-crash-offsetLeft.html:28
> +            if (window.testRunner) 

nit: you can just call endTest() function in full-screen-test.js

> LayoutTests/fullscreen/full-screen-crash-offsetLeft.html:37
> +        if (window.eventSender)

nit: you can use the runKeyDown function if you want.
Comment 4 Tony Chang 2012-06-22 15:46:11 PDT
Comment on attachment 149119 [details]
Proposed fix. NULL-check parent.

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

> LayoutTests/fullscreen/full-screen-crash-offsetLeft.html:5
> +    font-family: ahem;
> +    -webkit-font-smoothing: none;
> +    visibility: hidden;

Nit: Are these styles needed?
Comment 5 Julien Chaffraix 2012-06-22 16:02:38 PDT
Created attachment 149127 [details]
Patch for landing
Comment 6 Julien Chaffraix 2012-06-22 16:05:01 PDT
Comment on attachment 149119 [details]
Proposed fix. NULL-check parent.

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

I ended up rewriting the test to use the helpers. The output should be way better. You can have a look at the new patch to tell me if you are satisfied.

Thanks for the review, Abhishek and Tony!

>> LayoutTests/fullscreen/full-screen-crash-offsetLeft.html:5
>> +    visibility: hidden;
> 
> Nit: Are these styles needed?

Removed. I thought they were needed but not really.
Comment 7 WebKit Review Bot 2012-06-22 16:29:49 PDT
Comment on attachment 149127 [details]
Patch for landing

Clearing flags on attachment: 149127

Committed r121072: <http://trac.webkit.org/changeset/121072>
Comment 8 WebKit Review Bot 2012-06-22 16:30:09 PDT
All reviewed patches have been landed.  Closing bug.