Bug 62324 – [Chromium] Crash when closing a tab with accelerated 2d canvas

• Component: Canvas
• Status: RESOLVED FIXED

[Justin Novosad]

This is in reference to chromium bug 85309

Repro steps posted here:
http://code.google.com/p/chromium/issues/detail?id=85309

This is a Chrome release blocker

[Justin Novosad]

Created attachment 96493 [details]
Patch

[Justin Novosad]

No need to worry about cr-* test builds that will fail.  This patch depends on a skia change that has not yet rolled in.  Dependent change is pending review here:
http://codereview.appspot.com/4584048/

[Stephen White]

This looks ok to me, but jamesr and brian should take a look.

[James Robinson]

Comment on attachment 96493 [details]
Patch

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

You need to explain what's going on here, preferably in the ChangeLog.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This will fail an svn presubmit

[Justin Novosad]

Created attachment 96497 [details]
Patch

[Brian Salomon]

Comment on attachment 96497 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:126
> +        GrSafeUnref(m_grContext);

Minor: GrSafeUnref is now redundant, can be m_grContext->unref();

[James Robinson]

Comment on attachment 96497 [details]
Patch

Are the two fixes for the same thing or not?

[WebKit Review Bot]

Comment on attachment 96497 [details]
Patch

Attachment 96497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8776842

[Justin Novosad]

(In reply to comment #7)
> (From update of attachment 96497 [details])
> Are the two fixes for the same thing or not?

It fixes the same test case.  The callback was the first thing to crash. When I fixed that, there was another crash waiting behind it.

[Justin Novosad]

The skia change that fixes the build failure reported by the review bot is now in ToT skia:
http://code.google.com/p/skia/source/detail?r=1551

[James Robinson]

You need to update the skia rev in Source/WebKit/chromium/DEPS before landing this.  You also need to be sure the DEPS file in chromium trunk is updated before landing this.

Ping this bug again for review after you've done those two.  This can't land before then.

[James Robinson]

Comment on attachment 96497 [details]
Patch

R- until DEPS rolls, please flip back to review? once they've rolled in.

[Vangelis Kokkevis]

(In reply to comment #12)
> (From update of attachment 96497 [details])
> R- until DEPS rolls, please flip back to review? once they've rolled in.

James, can you r+ this and I'll cq+ once the skia deps roll lands?

[James Robinson]

Sure, update the bug when the rolls land.  Where are the bugs/codereviews/whatever for the rolls?

[Vangelis Kokkevis]

(In reply to comment #14)
> Sure, update the bug when the rolls land.  Where are the bugs/codereviews/whatever for the rolls?

Skia rolled past 1551 which is what was needed for this CL to land

http://src.chromium.org/viewvc/chrome?view=rev&revision=88481

[James Robinson]

It has to roll here as well or this will break bots: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS

should just need to increment chromium_rev to 88481 and make sure that already builds.  I'll start the process for that if nobody else has started it yet.

[James Robinson]

Rev at https://bugs.webkit.org/show_bug.cgi?id=62353, seeing how the EWS bots like it.

[James Robinson]

Roll going through EWS here: https://bugs.webkit.org/show_bug.cgi?id=62353

[James Robinson]

Comment on attachment 96497 [details]
Patch

OK, Source/WebKit/chromium/DEPS roll landed (hope it sticks)

[WebKit Review Bot]

Comment on attachment 96497 [details]
Patch

Clearing flags on attachment: 96497

Committed r88429: <http://trac.webkit.org/changeset/88429>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

drovering...