[Webkit-unassigned] [Bug 95057] [chromium] Allow impl-thread deletion of only some resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95057





--- Comment #5 from Antoine Labour <piman at chromium.org>  2012-08-27 11:34:44 PST ---
(From update of attachment 160669)
View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:120
> +        return m_backing->resourceDeleted();

Isn't this racy if the m_backingsMutex in the manager isn't held?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:230
> +            resourceProvider->deleteResource((*it)->id());

What if resourceProvider is NULL? Or, if the semantics have changed to not allow NULL anymore, mind adding an assert to make that clear?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:271
> +                resourcesToDelete.append((*it)->id());

It would be good to reset the resource ID here, making it harder to accidentally use a stale resource id.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:352
> +    ASSERT(backing->resourceDeleted() || !backing->owner() || !backing->owner()->isAbovePriorityCutoff());

since backing->resourceDeleted() has to be true (since you're asserting below), !backing->owner() || !backing->owner()->isAbovePriorityCutoff() is never tested. Should it be tested separately? Is it not true anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:143
> +    BackingSet m_backingsFreedByImplThread;

TBH a vector is just as good, and cheaper memory and time complexity-wise. You'll never add the same backing to this set twice, correct?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:151
> +    Mutex m_backingsMutex;

I think we should be clear what is protected by this mutex and what is not. It looks to me that what is actually protected by the mutex is:
- m_backings
- m_backingsFreedByImplThread
- the contents of each Backing in m_backings (in particular the m_resourceDeleted flag and the resource ID).
(basically anything that is accessed by deleteBackingsResourcesOnImplThread while the mutex is held).

Among others, what is not protected by this is:
- m_textures
- texture->backing() for each texture in m_texture.
It's important to note, so that we don't try to take the mutex in various operations and expect to be safe - we need the higher level notion of blocked main thread to ensure safety.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateQueue.cpp:69
> +        if (!upload.texture->backingWasAsynchronouslyDeleted())

Isn't this racy if the m_backingsMutex isn't held?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.