Check for the layoutObject before derefencing it.

Check for the layoutObject before derefencing it.

BUG=582149
Committed: https://crrev.com/6d9bebe9347d1e5bef73c3e806258f0ee050fed2
Cr-Commit-Position: refs/heads/master@{#372405}

[lfg, 2016-01-28 23:27:19]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2016-01-28 23:27:20]

Daniel, please take a look.

[lfg, 2016-01-28 23:44:50]

lfg@chromium.org changed reviewers:
+ piman@chromium.org

[lfg, 2016-01-28 23:44:50]

Hi piman, can you take a look? If you have a suggestion on how to improve the
test, in particular the wait until the frames are done, it could help make the
test better.

[dcheng, 2016-01-29 00:32:53]

Blink changes LGTM but with a question.

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:816: if (!owner ||
!owner->layoutObject() || !owner->layoutObject()->style())
Just for my understanding: when is LayoutObject::style() null?

[dcheng, 2016-01-29 00:35:35]

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:816: if (!owner ||
!owner->layoutObject() || !owner->layoutObject()->style())
On 2016/01/29 at 00:32:53, dcheng wrote:
> Just for my understanding: when is LayoutObject::style() null?

OK, per Elliot, there's no need to have a null check for style() here.

Since we're in a bit of a time crunch here to make canary, I'm going to dcommit
the fix without this null check.

[lfg, 2016-01-29 01:00:50]

On 2016/01/29 00:35:35, dcheng wrote:
>
https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:816: if (!owner ||
> !owner->layoutObject() || !owner->layoutObject()->style())
> On 2016/01/29 at 00:32:53, dcheng wrote:
> > Just for my understanding: when is LayoutObject::style() null?
> 
> OK, per Elliot, there's no need to have a null check for style() here.
> 
> Since we're in a bit of a time crunch here to make canary, I'm going to
dcommit
> the fix without this null check.

I thought this was necessary when I was printing m_layoutObject->style() and it
was
null, but it turns out this is inside a union{}, so I wasn't looking at the
correct
object.

[piman, 2016-01-29 01:07:50]

https://codereview.chromium.org/1648113002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1648113002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5187: // frames.
Agreed :)
Frames come via RenderWidgetHostViewBase::DidReceiveRendererFrame. Maybe we
could add a WaitForRendererFrameForTesting() there, that would do a nested loop
woken up in DidReceiveRendererFrame after renderer_frame_number_ is incremented?

[lfg, 2016-01-29 01:08:13]

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1648113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:816: if (!owner ||
!owner->layoutObject() || !owner->layoutObject()->style())
On 2016/01/29 00:32:53, dcheng wrote:
> Just for my understanding: when is LayoutObject::style() null?

Done.

[lfg, 2016-01-29 01:10:45]

https://codereview.chromium.org/1648113002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1648113002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5187: // frames.
On 2016/01/29 01:07:50, piman (OOO back 01-28) wrote:
> Agreed :)
> Frames come via RenderWidgetHostViewBase::DidReceiveRendererFrame. Maybe we
> could add a WaitForRendererFrameForTesting() there, that would do a nested
loop
> woken up in DidReceiveRendererFrame after renderer_frame_number_ is
incremented?

The problem here is figuring out how many frames do I need to wait until the
renderer has finished rendering the first frame after the last javascript call.

I was thinking that maybe the renderer could read back the screen to force a
synchronization with the browser?

[lfg, 2016-01-29 20:18:44]

I talked with Daniel and Charlie, and we are going to land the fix now so it
unblocks the top crasher, and I'll keep working on the test.

[lfg, 2016-01-29 20:18:49]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-01-29 20:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1648113002/#ps60001
(title: "remove test")

[commit-bot: I haz the power, 2016-01-29 20:20:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1648113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1648113002/60001

[commit-bot: I haz the power, 2016-01-29 20:27:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-29 20:30:03]

Description was changed from

==========
Check for the layoutObject before derefencing it.

BUG=582149
==========

to

==========
Check for the layoutObject before derefencing it.

BUG=582149

Committed: https://crrev.com/6d9bebe9347d1e5bef73c3e806258f0ee050fed2
Cr-Commit-Position: refs/heads/master@{#372405}
==========

[commit-bot: I haz the power, 2016-01-29 20:30:03]

Patchset 4 (id:??) landed as
https://crrev.com/6d9bebe9347d1e5bef73c3e806258f0ee050fed2
Cr-Commit-Position: refs/heads/master@{#372405}