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.
https://bugs.webkit.org/show_bug.cgi?id=77135 runs into the same issue. Alok said he had weak pointer behavior working locally.
Created attachment 125254 [details] proposed patch
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 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 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 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.
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.
Hmm, when patching this in locally there seems to be a lot of memory corruption running the unit tests. Will investigate.
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
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.
Created attachment 125434 [details] Patch
I think this should work, and have added a unit test.
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.
(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 on attachment 125434 [details] Patch Looks good!
Comment on attachment 125434 [details] Patch rs=me based on others' reviews.
Committed r106840: <http://trac.webkit.org/changeset/106840>
Comment on attachment 125434 [details] Patch Patch landed, clearing flags