Bug 89785 – REGRESSION(r116446): Crash in RenderBoxModelObject::adjustedPositionRelativeToOffsetParent

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 149119 [details]
Proposed fix. NULL-check parent.

[Abhishek Arya]

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.

[Abhishek Arya]

typo it is runWithKeyDown (http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fullscreen/full-screen-test.js&exact_package=chromium&q=runWithKeyDown&type=cs&l=12)

[Tony Chang]

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?

[Julien Chaffraix]

Created attachment 149127 [details]
Patch for landing

[Julien Chaffraix]

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.

[WebKit Review Bot]

Comment on attachment 149127 [details]
Patch for landing

Clearing flags on attachment: 149127

Committed r121072: <http://trac.webkit.org/changeset/121072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.