Bug 77655 – [chromium] Support detaching TextureManager from ManagedTexture

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

[Ami Fischman]

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.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=77135 runs into the same issue.  Alok said he had weak pointer behavior working locally.

[Alok Priyadarshi]

Created attachment 125254 [details]
proposed patch

[Alok Priyadarshi]

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.

[Ami Fischman]

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?

[James Robinson]

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.

[Alok Priyadarshi]

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.

[Alok Priyadarshi]

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.

[James Robinson]

Hmm, when patching this in locally there seems to be a lot of memory corruption running the unit tests. Will investigate.

[James Robinson]

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

[James Robinson]

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.

[James Robinson]

Created attachment 125434 [details]
Patch

[James Robinson]

I think this should work, and have added a unit test.

[Alok Priyadarshi]

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.

[James Robinson]

(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.

[Vangelis Kokkevis]

Comment on attachment 125434 [details]
Patch

Looks good!

[Kenneth Russell]

Comment on attachment 125434 [details]
Patch

rs=me based on others' reviews.

[James Robinson]

Committed r106840: <http://trac.webkit.org/changeset/106840>

[James Robinson]

Comment on attachment 125434 [details]
Patch

Patch landed, clearing flags