Bug 101947 – Don't mark scrolling contents as dirty if RenderLayerBacking is going away

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sami Kyöstilä]

Don't mark scrolling contents as dirty if RenderLayerBacking is going away

[Sami Kyöstilä]

Created attachment 173663 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 173663 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        track repaints or not. If the RenderLayerBacking is being destroyed, the
> +        compositor is no longer valid and this causes a crash.

This seems like the wrong way to fix the crash. Does 'no longer valid' mean null, or garbage? if the latter, we need to null out a pointer somewhere.

[Sami Kyöstilä]

(In reply to comment #2)
> This seems like the wrong way to fix the crash. Does 'no longer valid' mean null, or garbage? if the latter, we need to null out a pointer somewhere.

Specifically, renderer()->view() is null, so we get a crash (and an assertion failure) in RenderLayer::compositor().

An alternative way to fix this would be to make RenderLayerBacking::isTrackingRepaints() check for a valid view before calling compositor().

[Sami Kyöstilä]

Created attachment 173923 [details]
Patch

Alternative fix.

[Simon Fraser (smfr)]

Comment on attachment 173923 [details]
Patch

I still don't understand. You say compositor is no longer valid. Does this mean that RenderLayerBacking::compositor() returns garbage? Or null?

[Sami Kyöstilä]

(In reply to comment #5)
> (From update of attachment 173923 [details])
> I still don't understand. You say compositor is no longer valid. Does this mean that RenderLayerBacking::compositor() returns garbage? Or null?

It returns garbage, because it tries to get the compositor from a null RenderView. There's a debug assert in RenderLayer::compositor() to guard against this.

[Simon Fraser (smfr)]

So let's fix that (returning garbage is a security issue, returning null may crash, but safely). I don't think we want to add null checks to every call to compositor(), so your current patch is still needed.

[Sami Kyöstilä]

(In reply to comment #7)
> So let's fix that (returning garbage is a security issue, returning null may crash, but safely). I don't think we want to add null checks to every call to compositor(), so your current patch is still needed.

Agreed. Actually RenderLayerBacking::isTrackingRepaints() already checks for a null compositor so no changes are needed there.

[Sami Kyöstilä]

Created attachment 173941 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173941 [details]
Patch

Rejecting attachment 173941 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Clean up the inheritance tree under the MediaControls Class.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14826313

[Sami Kyöstilä]

Created attachment 174002 [details]
Patch

Weird. That merge conflict is from a completely unrelated patch. Here's a rebased version just in case.

[WebKit Review Bot]

Comment on attachment 174002 [details]
Patch

Clearing flags on attachment: 174002

Committed r134536: <http://trac.webkit.org/changeset/134536>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.