Bug 77655 - [chromium] Support detaching TextureManager from ManagedTexture
Summary: [chromium] Support detaching TextureManager from ManagedTexture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks: 77135
  Show dependency treegraph
 
Reported: 2012-02-02 09:56 PST by Ami Fischman
Modified: 2012-02-06 16:17 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (8.20 KB, patch)
2012-02-02 21:19 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2012-02-03 15:35 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-02-02 09:56:06 PST
ManagedTexture stores a raw pointer to its TextureManager, so if a TextureManager is deleted, subsequent attempts to use the ManagedTexture behave erratically (mostly either crash or infinitely-loop as the now-deleted manager's hashtable is used to look for the texture's token).

As a result ManagedTexture's can't be cached/reused (see bug 77654 for one example where this bites <video>).

It would be nice if ManagedTexture could be made robust to its TextureManager being deleted.
Comment 1 James Robinson 2012-02-02 16:31:10 PST
https://bugs.webkit.org/show_bug.cgi?id=77135 runs into the same issue.  Alok said he had weak pointer behavior working locally.
Comment 2 Alok Priyadarshi 2012-02-02 21:19:28 PST
Created attachment 125254 [details]
proposed patch
Comment 3 Alok Priyadarshi 2012-02-02 21:21:08 PST
Comment on attachment 125254 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/TextureManager.h:114
> +    // FIXME: Now that we also store references to ManagedTexture

I made some progress on this but decided against making this change for M18.
Comment 4 Ami Fischman 2012-02-03 10:38:46 PST
Comment on attachment 125254 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
> +    m_textureManager = 0;

Move this into the body of reset() so steal() also gets it?
Comment 5 James Robinson 2012-02-03 11:00:46 PST
Comment on attachment 125254 [details]
proposed patch

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

Very close.

I'd really like to see unit tests for this. Specifically, construct TextureManager / ManagedTexture pairs and destroy them in both orders to verify that we null out the right pointers and don't crash.

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
>> +    m_textureManager = 0;
> 
> Move this into the body of reset() so steal() also gets it?

Yes, you definitely need to do this

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:49
> +    const TextureManager* manager() const { return m_textureManager; }

why do you need to expose a getter for this? that doesn't seem helpful

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:50
> +    void managerWillDie();

a more idiomatic name for this would be "clearManager()", it's what we use elsewhere.

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:139
> +    // Finding in reverse should be in general faster in this case.
> +    // Textures for dynamic layers should be newer compared to those for
> +    // static layers and hence at the end of the array.

If you use a set then this goes away

> Source/WebCore/platform/graphics/chromium/TextureManager.h:112
> +    Vector<ManagedTexture*> m_registeredTextures;

this should be a HashSet<>. We don't care about order but we do care about fast existence checks.
Comment 6 Alok Priyadarshi 2012-02-03 14:30:05 PST
Comment on attachment 125254 [details]
proposed patch

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

>>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
>>> +    m_textureManager = 0;
>> 
>> Move this into the body of reset() so steal() also gets it?
> 
> Yes, you definitely need to do this

But there is no need to NULL m_textureManager when doing steal(). The texture can still be reused by calling reserve(). Also (in my opinion) reset should restore the state right after construction.

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:49
>> +    const TextureManager* manager() const { return m_textureManager; }
> 
> why do you need to expose a getter for this? that doesn't seem helpful

I was expecting to use it for another bug where I needed to compare texture-mgr pointers. But it can be taken out from this patch.

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:50
>> +    void managerWillDie();
> 
> a more idiomatic name for this would be "clearManager()", it's what we use elsewhere.

Ok.

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:139
>> +    // static layers and hence at the end of the array.
> 
> If you use a set then this goes away

Sure

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:112
>> +    Vector<ManagedTexture*> m_registeredTextures;
> 
> this should be a HashSet<>. We don't care about order but we do care about fast existence checks.

Sounds reasonable.
Comment 7 Alok Priyadarshi 2012-02-03 14:31:06 PST
I am OOO today, but may get some time tonight to address these suggestions. If you cannot wait until then feel free to take it over.
Comment 8 James Robinson 2012-02-03 14:37:17 PST
Hmm, when patching this in locally there seems to be a lot of memory corruption running the unit tests. Will investigate.
Comment 9 James Robinson 2012-02-03 14:54:46 PST
Comment on attachment 125254 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:59
> +    if (m_token && m_textureManager) {
>          m_textureManager->releaseToken(m_token);
> +        m_textureManager->unregisterTexture(this);

this is crash city - you need to unregister ManagedTextures even if m_token is zero
Comment 10 James Robinson 2012-02-03 15:02:47 PST
Actually, reset()'ing when the TextureManager is cleared doesn't make much sense either - isValid() and reserve() will return false when the manager is null so it doesn't matter what the values are set to.  When we support setting a new TextureManager then it will make sense to reset the appropriate fields then.

I'm going to fix some of the more egregious problems with this patch (did you even run the unit tests, Alok?), add some tests, and upload a new version based off of Alok's patch.
Comment 11 James Robinson 2012-02-03 15:35:16 PST
Created attachment 125434 [details]
Patch
Comment 12 James Robinson 2012-02-03 15:36:07 PST
I think this should work, and have added a unit test.
Comment 13 Alok Priyadarshi 2012-02-03 15:41:56 PST
Comment on attachment 125434 [details]
Patch

LGTM

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

> Source/WebCore/platform/graphics/chromium/TextureManager.h:116
>      typedef HashMap<TextureToken, TextureInfo> TextureMap;

I think we can now get rid of this TextureMap. I had added a FIXME in the original patch about this.
Comment 14 James Robinson 2012-02-03 15:42:52 PST
(In reply to comment #13)
> (From update of attachment 125434 [details])
> LGTM
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=125434&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TextureManager.h:116
> >      typedef HashMap<TextureToken, TextureInfo> TextureMap;
> 
> I think we can now get rid of this TextureMap. I had added a FIXME in the original patch about this.

I suspect this is true, but it's a larger change.  Thanks for looking.
Comment 15 Vangelis Kokkevis 2012-02-06 12:15:30 PST
Comment on attachment 125434 [details]
Patch

Looks good!
Comment 16 Kenneth Russell 2012-02-06 12:52:02 PST
Comment on attachment 125434 [details]
Patch

rs=me based on others' reviews.
Comment 17 James Robinson 2012-02-06 13:18:03 PST
Committed r106840: <http://trac.webkit.org/changeset/106840>
Comment 18 James Robinson 2012-02-06 16:17:36 PST
Comment on attachment 125434 [details]
Patch

Patch landed, clearing flags