Bug 47197 - Fix ownership of GraphicsContext3D in SharedGraphicsContext3D to prevent early deallocation and crash
Summary: Fix ownership of GraphicsContext3D in SharedGraphicsContext3D to prevent earl...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Chris Marrin
Depends on:
Reported: 2010-10-05 11:05 PDT by Chris Marrin
Modified: 2010-12-20 22:52 PST (History)
4 users (show)

See Also:

Patch (1.50 KB, patch)
2010-10-05 11:07 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (2.34 KB, patch)
2010-10-05 11:15 PDT, Chris Marrin
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-10-05 11:05:35 PDT
Fix ownership of GraphicsContext3D in SharedGraphicsContext3D to prevent early deallocation and crash
Comment 1 Chris Marrin 2010-10-05 11:07:32 PDT
Created attachment 69812 [details]
Comment 2 Adam Roben (:aroben) 2010-10-05 11:10:22 PDT
Comment on attachment 69812 [details]

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

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:59
> -    OwnPtr<GraphicsContext3D> context = GraphicsContext3D::create(attr, hostWindow);
> +    PassOwnPtr<GraphicsContext3D> context = GraphicsContext3D::create(attr, hostWindow);
>      if (!context)
>          return 0;
> -    return adoptRef(new SharedGraphicsContext3D(context.get()));
> +    return adoptRef(new SharedGraphicsContext3D(context));
>  }

It would be better to keep the context variable an OwnPtr, and pass context.release() to SharedGraphicsContext3D.

Is it possible to make a test?
Comment 3 Chris Marrin 2010-10-05 11:15:27 PDT
Created attachment 69814 [details]
Comment 4 Adam Roben (:aroben) 2010-10-05 11:17:01 PDT
Comment on attachment 69814 [details]

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

r=me if you remove the FeatureDefines change.

> WebCore/ChangeLog:10
> +        This is work in progress and the crash only happens with ACCELERATED_2D_CANVAS turned on (which is off
> +        by default). So no test cases yet.
> +

It would be better to say that many existing test cases will crash if ACCELERATED_2D_CANVAS is turned on. (If that were not the case, you could conceivably write a test case that would crash once it was turned on. But since we already have such test cases, you're off the hook!)

> WebCore/Configurations/FeatureDefines.xcconfig:39

Comment 5 Eric Seidel (no email) 2010-12-14 01:54:39 PST
Was this landed and just not closed?
Comment 6 Eric Seidel (no email) 2010-12-20 22:52:53 PST
r69127.  Please close bugs after landing or use a tool which does it for you (webkit-patch land for example).