| Summary: | adding support for DiscardablePixelRef for caching lazily decoded images | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Min Qin <qinmin> | ||||||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, cc-bugs, dglazkov, eric, fishd, hclam, jamesr, keishi, levin+threading, nduca, noel.gordon, peter+ews, senorblanco, tkent+wkapi, wangxianzhu, webkit.review.bot | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 108143 | ||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
Min Qin
2013-01-14 16:28:00 PST
Created attachment 182657 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Thanks for the patch. I'll look. Comment on attachment 182657 [details] Patch Attachment 182657 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15884365 Comment on attachment 182657 [details]
Patch
Isn't there a way to remove all these modes and stuff by
0) always delegating allocator up to skia, e.g. instead of skia::DiscardableMemoryAllocator, use skia::MemoryAllocator
1) basically making the cache size a webkit preference that we set up in the embedder, and
2) making the lazy pixel ref have a mode saying "are you purged"? eg asume all lazy refs can be purged/lost then just deal with it? The thing is, other than the static cast here, and the size set differently, there's nothign much special webkit side about this...Isn't there a way to remove all these modes and stuff by
0) always delegating allocator up to skia, e.g. instead of skia::DiscardableMemoryAllocator, use skia::MemoryAllocator
1) basically making the cache size a webkit preference that we set up in the embedder, and
2) making the lazy pixel ref have a mode saying "are you purged"? eg asume all lazy refs can be purged/lost then just deal with it? The thing is, other than the static cast here, and the size set differently, there's nothign much special webkit side about this...
Comment on attachment 182657 [details] Patch Attachment 182657 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15872680 (In reply to comment #5) > (From update of attachment 182657 [details]) > Isn't there a way to remove all these modes and stuff by > 0) always delegating allocator up to skia, e.g. instead of skia::DiscardableMemoryAllocator, use skia::MemoryAllocator Not all the SkPixelRef will use DiscardableMemory, we have to pass a flag from webkit to skia. In thise case, we use MemoryAllocator as the flag. If we pass a flag to MemoryAllocator, then we have to change all the third_party/skia code. > 1) basically making the cache size a webkit preference that we set up in the embedder, and > 2) making the lazy pixel ref have a mode saying "are you purged"? eg asume all lazy refs can be purged/lost then just deal with it? The thing is, other than the static cast here, and the size set differently, there's nothign much special webkit side about this... Lazy_pixel_ref has no idea whether the memory was purged. Only DiscardablePixelRef knows it. LazyPixelRef was created as a place holder, so it doesn't know the underneath implementation. However， the LazyDecodingPixelRef::prepareToDecode() is already doing sth. similar to what you are saying. Created attachment 183206 [details]
Patch
Comment on attachment 183206 [details] Patch Attachment 183206 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15906980 Comment on attachment 183206 [details] Patch Attachment 183206 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15907912 Created attachment 183223 [details]
Patch
Added the chromium CL as reference in the changeLog. I feel like other platforms have discardable buffer w/o webcore having to know about it. It's not imediately clear to me why this is needed in WebCore. Comment on attachment 183223 [details] Patch Attachment 183223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15944061 (In reply to comment #13) > I feel like other platforms have discardable buffer w/o webcore having to know about it. It's not imediately clear to me why this is needed in WebCore. We want to use discardable memory to back the memory that the WebCore image decoders use, which means that at some level this needs to be hooked into WebCore. Comment on attachment 183223 [details] Patch Attachment 183223 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15941163 carving off WebDiscardable to https://bugs.webkit.org/show_bug.cgi?id=107183 Comment on attachment 183223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review > Source/Platform/chromium/public/Platform.h:-160 > - nit: Don't remove this line. > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:65 > + return m_discardable; Is it possible that after this is allocated that it is immediately discard before lock? I think allocateAndLock has to be one operation. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:39 > + nit: No need to add empty lines. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:114 > + if (m_discardableMemoryEnabled && !(*cachedImage)->bitmap().pixelRef()) { Is this correct? I think SkBitmap always has a pointer to SkPixelRef but SkBitmap::getPixels() can return 0 because lock on SkPixelRef failed. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:115 > + removeCacheEntry(cacheEntry); I believe you have to unlock the SkBitmap no matter what, before you can remove it. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:-114 > - nit: Don't remove empty lines. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:76 > + bool discardableMemoryEnabled() { return m_discardableMemoryEnabled; } I would like to see some unit tests, even if it is just for Android. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:-202 > - nit: Don't remove empty lines. Sorry for the delay. I looked through the code and the approach seems right to me. I would like see another pass with DiscardablePixelRef taken out as a separate patch. Thanks. Comment on attachment 183223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > + dst->lockPixels(); I think this lockPixels() is not balanced by any unlockPixels(). I need to think about how to count all lock/unlock for ASSERT. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:118 > + scaledBitmap = skia::ImageOperations::Resize(fullSizeImage->bitmap(), resizeMethod(), scaledSize.width(), scaledSize.height(), &allocator); Please add a comment of what this call entails. This call allocates DiscardablePixelRef, pin and then unpin. Which means using scaledBitmap can be discarded immediately after this call so warn it here. One thing I'm not clear on is why we need a discardable pixelref, as well as one that does deferred decoding. Shouldn't the deferred decoding one simply allocate discardable memory onLockPixels()? (One PixelRef to rule them all..) Or is this for cacheing resized subsets? Comment on attachment 183223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:65 >> + return m_discardable; > > Is it possible that after this is allocated that it is immediately discard before lock? I think allocateAndLock has to be one operation. +1 to that. Could this be done in the onLockPixels() somehow? > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:73 > + if (m_discardable) > + m_lockedMemory = m_discardable->lock(); > + *ctable = m_colorTable; > + return m_lockedMemory; This looks like it's the only place m_lockedMemory is assigned, and it'll return garbage if m_discardable is NULL. Am I missing something? >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:114 >> + if (m_discardableMemoryEnabled && !(*cachedImage)->bitmap().pixelRef()) { > > Is this correct? I think SkBitmap always has a pointer to SkPixelRef but SkBitmap::getPixels() can return 0 because lock on SkPixelRef failed. SkBitmap can exist without a PixelRef (when the SkBitmap owns its own memory directly). Not sure if that'll happen in this particular case, but it's definitely a supported semantic. This actually confused me for a while too. Let me attempt to explain as I got to understanding it.... maybe the way I understand this isn't how the patch panned out, but its what I have in my head:
1. WebCore paint inserts SkBitmaps(LazyPixelRef) into the deferred canvas
2. When we rasterize the canvas, we discover lazy pixel ref and go find its CacheEntries. Recall, there may be multiple, because each CacheEntry is for a different decode/resize scale.
Thus, you may have the following situation for a single bitmap inserted into the deferred canvas:
foo.png <Lazy, raw size 512x512>
CacheEntry: foo.png <decoded to 128x128, then scaled to 100x100>
CacheEntry: foo.png <decoded to 512x512, not scaled>
The cache entry need to store each decoded bitmap data, plus its size/stride/blahblah. So, void* plus a bunch of fields... instead of duplicating those fields, we use a SkBitmap.
Since this cache entry bitmap is the one omnomnoming at memory, its the one we want to die a violent death under memory pressure. So, we replace the SkBitmap-backed-by-void* with a SkBitmap-backed-by-a-WebDiscardable. In an 2D api that used subclassing, the cache entry would have a SkDiscardableBitmap and it'd be easier to grok.
(In reply to comment #23) > This actually confused me for a while too. Let me attempt to explain as I got to understanding it.... maybe the way I understand this isn't how the patch panned out, but its what I have in my head: > > 1. WebCore paint inserts SkBitmaps(LazyPixelRef) into the deferred canvas > 2. When we rasterize the canvas, we discover lazy pixel ref and go find its CacheEntries. Recall, there may be multiple, because each CacheEntry is for a different decode/resize scale. > > Thus, you may have the following situation for a single bitmap inserted into the deferred canvas: > > foo.png <Lazy, raw size 512x512> > CacheEntry: foo.png <decoded to 128x128, then scaled to 100x100> > CacheEntry: foo.png <decoded to 512x512, not scaled> > > > The cache entry need to store each decoded bitmap data, plus its size/stride/blahblah. So, void* plus a bunch of fields... instead of duplicating those fields, we use a SkBitmap. > > Since this cache entry bitmap is the one omnomnoming at memory, its the one we want to die a violent death under memory pressure. So, we replace the SkBitmap-backed-by-void* with a SkBitmap-backed-by-a-WebDiscardable. In an 2D api that used subclassing, the cache entry would have a SkDiscardableBitmap and it'd be easier to grok. Pretty much! Here's another view: As recorded the relationship of classes: SkPicture -> SkBitmap -> SkPixelRef (LazyDecodingPixelRef) When rasterizing LazyDecodingPixelRef, it goes like this: LazyDecodingPixelRef -> ImageDecodingStore -> CacheEntry -> ScaledImageFragment -> (*) SkBitmap (actually carry the decoded bits) We would like to make the SkBitmap, marked by (*) as discardable on capable platforms. So we replace the guts of (*) SkBitmap such that it contains: (*) SkBitmap -> DiscardablePixelRef -> WebDiscardable In the non-discardable case (i.e. normal case), SkBitmap has the following content: (*) SkBitmap -> SkMallocPixelRef -> void* (a piece of memory) Makes sense. Thanks for the explanation(s)! Created attachment 183528 [details]
Patch
Comment on attachment 183223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review >> Source/Platform/chromium/public/Platform.h:-160 >> - > > nit: Don't remove this line. Done >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 >> + dst->lockPixels(); > > I think this lockPixels() is not balanced by any unlockPixels(). > > I need to think about how to count all lock/unlock for ASSERT. Removed the lockPixels() call here. We don't need this for DiscardablePixelRef, the memory should be pinned after allocDiscardableMemory(). I think there are some confusion here： lock the pixelref and pin the memory. The pixelref doesn't needs to be locked, but the memory needs to be pinned so imagede coders can write to it. Later on, the first lockPixels() call will always succeed and pin the memory, and the unlockPixels() call will unlock and unpin the memory. >>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:65 >>> + return m_discardable; >> >> Is it possible that after this is allocated that it is immediately discard before lock? I think allocateAndLock has to be one operation. > > +1 to that. Could this be done in the onLockPixels() somehow? This call should lock the pixelref and pin the memory. We don't want to move this into onLockPixels as then we have to deal with lock/unlock balances initially. And since we are not calling dst->lockPixels(), so we should not move this into onLockPixels(). >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:73 >> + return m_lockedMemory; > > This looks like it's the only place m_lockedMemory is assigned, and it'll return garbage if m_discardable is NULL. Am I missing something? good catch, I should set m_lockedMemory to NULL if m_discardable is NULL. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:39 >> + > > nit: No need to add empty lines. Done >>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:114 >>> + if (m_discardableMemoryEnabled && !(*cachedImage)->bitmap().pixelRef()) { >> >> Is this correct? I think SkBitmap always has a pointer to SkPixelRef but SkBitmap::getPixels() can return 0 because lock on SkPixelRef failed. > > SkBitmap can exist without a PixelRef (when the SkBitmap owns its own memory directly). Not sure if that'll happen in this particular case, but it's definitely a supported semantic. You are right, this seems a bug, should be getPixels(). >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:115 >> + removeCacheEntry(cacheEntry); > > I believe you have to unlock the SkBitmap no matter what, before you can remove it. Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:-114 >> - > > nit: Don't remove empty lines. Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:76 >> + bool discardableMemoryEnabled() { return m_discardableMemoryEnabled; } > > I would like to see some unit tests, even if it is just for Android. Not sure unittest worth it here. I have added some unittest in base/. >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:118 >> + scaledBitmap = skia::ImageOperations::Resize(fullSizeImage->bitmap(), resizeMethod(), scaledSize.width(), scaledSize.height(), &allocator); > > Please add a comment of what this call entails. > > This call allocates DiscardablePixelRef, pin and then unpin. Which means using scaledBitmap can be discarded immediately after this call so warn it here. Done >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:-202 >> - > > nit: Don't remove empty lines. Done PTAL. This change now depends on https://bugs.webkit.org/show_bug.cgi?id=107183 as WebDiscardableMemory code has been moved there. We can wait until that change got landed and then land this one Comment on attachment 183528 [details] Patch Attachment 183528 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15949465 Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ? Can we use that directly? If not, can we look at how it's implemented and learn anything? For instance, it seems to always round the allocation size up to the next page size. Comment on attachment 183528 [details] Patch Attachment 183528 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15939810 Comment on attachment 183223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review >>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 >>> + dst->lockPixels(); >> >> I think this lockPixels() is not balanced by any unlockPixels(). >> >> I need to think about how to count all lock/unlock for ASSERT. > > Removed the lockPixels() call here. We don't need this for DiscardablePixelRef, the memory should be pinned after allocDiscardableMemory(). > I think there are some confusion here： lock the pixelref and pin the memory. The pixelref doesn't needs to be locked, but the memory needs to be pinned so imagede coders can write to it. Later on, the first lockPixels() call will always succeed and pin the memory, and the unlockPixels() call will unlock and unpin the memory. I am wrong on this. We need this lockPixels() or otherwise it is not safe. When imagedecoder calls setSize(), skia uses a SkAutoLockPixels to lock and unlock the pixelRef. And after the pixelRef become unlocked, imagedecoder cannot access the memory again. (In reply to comment #30) > Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ? Can we use that directly? If not, can we look at how it's implemented and learn anything? For instance, it seems to always round the allocation size up to the next page size. I'd really like an answer to ^^ Created attachment 184061 [details]
Patch
I don't think we can easily use that class. 1. It is not threadsafe. It uses a mutex ring which is not threadsafe. 2. It uses SkImageDecoder, not sure how much effort we will have to maintain that later on. 3. For a single image, we can have multiple cache entries(different resolutions, rects.) And we have two PixelRefs. One placeholder(LazyDecodingPixelRef), one stores the pixel(DiscardablePixelRef). Decode() is called on a single placeholder, while storage is on different DiscardablePixelRefs. So there is a 1:M relationship here. I think we can add the allocation size limit here, but I am not sure whether we should have it in this patch. Maybe a follow up one. (In reply to comment #33) > (In reply to comment #30) > > Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ? Can we use that directly? If not, can we look at how it's implemented and learn anything? For instance, it seems to always round the allocation size up to the next page size. > > I'd really like an answer to ^^ Comment on attachment 184061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184061&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:42 > + // unlockPixels() will be called when the image decoder goes away. Does it mean that lock to DiscardablePixelRef is held for the entire lifetime of ImageDecoder? By the way I think your patch is mixed with the patch to fix race condition. Comment on attachment 184061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184061&action=review Uploading a new patch to include recent changes to WebDiscardableMemory and the fix for race conditions. PTAL >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:42 >> + // unlockPixels() will be called when the image decoder goes away. > > Does it mean that lock to DiscardablePixelRef is held for the entire lifetime of ImageDecoder? Yes. In ImageDecodingStore::insertAndLockCache(), when the decoder is going away, the SkBitmap associated with the imageDecoder will also gone. That's when the unlockPixels() gets called. Since ImageFrameGenerator::decode() assigned that SkBitmap to another Skbitmap(fullSizeBitmap), so the fullSizeBitmap will have its own count on lock/unlockpixels(). Created attachment 184272 [details]
Patch
Comment on attachment 184272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184272&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:55 > + delete pixelRef; instead of manually calling delete, can you put pixelRef in an OwnPtr<> and .leakPtr() it on line 49? > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > + m_discardable->lock(); don't you need to check the return value of lock() to see if the memory's still there before calling ->data() ? Created attachment 184379 [details]
Patch
Comment on attachment 184272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184272&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:55 >> + delete pixelRef; > > instead of manually calling delete, can you put pixelRef in an OwnPtr<> and .leakPtr() it on line 49? Done. >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 >> + m_discardable->lock(); > > don't you need to check the return value of lock() to see if the memory's still there before calling ->data() ? Since we have a DCHECK instead of returning a NULL on ->data() call after locking failure, we have to check this return value now. Code changed Created attachment 184554 [details]
Patch
(In reply to comment #38) > (From update of attachment 184061 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184061&action=review > > Uploading a new patch to include recent changes to WebDiscardableMemory and the fix for race conditions. PTAL > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:42 > >> + // unlockPixels() will be called when the image decoder goes away. > > > > Does it mean that lock to DiscardablePixelRef is held for the entire lifetime of ImageDecoder? > > Yes. In ImageDecodingStore::insertAndLockCache(), when the decoder is going away, the SkBitmap associated with the imageDecoder will also gone. That's when the unlockPixels() gets called. > Since ImageFrameGenerator::decode() assigned that SkBitmap to another Skbitmap(fullSizeBitmap), so the fullSizeBitmap will have its own count on lock/unlockpixels(). I'm still confused with this statement. SkBitmap doesn't call SkPixelRef::lockPixels() on its own, it is called only when needed isn't it? Let's have VC today, I don't want to block you for too long. Comment on attachment 184554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184554&action=review > Source/WebCore/ChangeLog:9 > + Instead of storing images in the memory, decoded images will be stored in DicardableMemory(such as ashmem). Please state clearly what is involved in this patch: - Allow use of discardable memory in deferred image decoding path. - Fully decoded images are unpinned and stored in ImageDecodingStore. - Partially decoded images are pinned and stored in ImageDecodingStore. - Separate caching policy for discardable memory - Separate cache size limit for discardable entries - Separate cache count limit for discardable entries > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51 > + dst->lockPixels(); Say this instead: This method is only called when a DiscardablePixelRef is created to back a SkBitmap. It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:113 > + if (!pixelRef->pixels()) { 1. Can we move this check before line 103 such that we only increment use count if pixelRef->pixels() return true? 2. Please also run removeCacheEntryInternal(cacheEntry) outside of the lock. You can have another success variable and return outside of the block. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:252 > + while (cacheEntry && isPruneNeeded()) { There should be 2 while loops, one goes through heap backed cache entry. One goes through discardable cache entry. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:269 > +bool ImageDecodingStore::needToPruneCacheEntry(const CacheEntry* cacheEntry) const See my comments for line 252. I don't think you need a separate function for this. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:201 > ImageFrame* frame = (*decoder)->frameBufferAtIndex(0); Add a comment after this line something like: If this call results in a new DiscardablePixelRef created, then ImageFrame::m_bitmap and DiscardablePixelRef contained is unlocked when ImageDecoder is destroyed. This is because ImageDecoder owns ImageFrame. We do not unlock ImaegFrame::m_bitmap, the consequence is that partially decoded SkBitmap will not unlocked when it is inserted into the ImageDecodingStore. We discussed thoroughly offline about this patch. We settled on the limitation of this patch, that is: 1. Fully decoded cache entries is unlocked and memory unpinned. 2. Partially decoded cache entries is not unlocked and memory stay pinned. Because there is a separate memory limit and cache count limit, the memory usage by partially decoded cache entries will not go unbounded. It is safe to accept this patch with above limitations. We agreed to work on a second iteration to complete the part for partially decoded cache entries. Comment on attachment 184554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184554&action=review >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:113 >> + if (!pixelRef->pixels()) { > > 1. Can we move this check before line 103 such that we only increment use count if pixelRef->pixels() return true? > > 2. Please also run removeCacheEntryInternal(cacheEntry) outside of the lock. You can have another success variable and return outside of the block. Sorry what I mean for 2 is to delete the CacheEntry outside the lock. I would merge removeCacheEntryInternal here and put the Vector<...> above the mutex. Comment on attachment 184554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184554&action=review >> Source/WebCore/ChangeLog:9 >> + Instead of storing images in the memory, decoded images will be stored in DicardableMemory(such as ashmem). > > Please state clearly what is involved in this patch: > > - Allow use of discardable memory in deferred image decoding path. > - Fully decoded images are unpinned and stored in ImageDecodingStore. > - Partially decoded images are pinned and stored in ImageDecodingStore. > - Separate caching policy for discardable memory > - Separate cache size limit for discardable entries > - Separate cache count limit for discardable entries Done >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51 >> + dst->lockPixels(); > > Say this instead: > > This method is only called when a DiscardablePixelRef is created to back a SkBitmap. It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef. Done >>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:113 >>> + if (!pixelRef->pixels()) { >> >> 1. Can we move this check before line 103 such that we only increment use count if pixelRef->pixels() return true? >> >> 2. Please also run removeCacheEntryInternal(cacheEntry) outside of the lock. You can have another success variable and return outside of the block. > > Sorry what I mean for 2 is to delete the CacheEntry outside the lock. I would merge removeCacheEntryInternal here and put the Vector<...> above the mutex. Done. Added a temp variable image, so we don't return it to the caller thru cachedImage when this if statement returns false. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:252 >> + while (cacheEntry && isPruneNeeded()) { > > There should be 2 while loops, one goes through heap backed cache entry. One goes through discardable cache entry. Done. Need another vector for this though. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:269 >> +bool ImageDecodingStore::needToPruneCacheEntry(const CacheEntry* cacheEntry) const > > See my comments for line 252. I don't think you need a separate function for this. Done >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:201 >> ImageFrame* frame = (*decoder)->frameBufferAtIndex(0); > > Add a comment after this line something like: > > If this call results in a new DiscardablePixelRef created, then ImageFrame::m_bitmap and DiscardablePixelRef contained is unlocked when ImageDecoder is destroyed. This is because ImageDecoder owns ImageFrame. We do not unlock ImaegFrame::m_bitmap, the consequence is that partially decoded SkBitmap will not unlocked when it is inserted into the ImageDecodingStore. Done Created attachment 184799 [details]
Patch
James, Stephen, would you please take a look at this change? BTW, there is a chromium change based on this: https://codereview.chromium.org/12077002/ James, would you please also take a look on that? Thanks Comment on attachment 184799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review > Source/WebKit/chromium/ChangeLog:8 > + Adding DiscardableMemory preferences to WebSettings and new tests for ImageDecodingStore Dont have a preference for this. Its either supported by the platform, or its not. Dont probe for support. Just try creating the discardable. It will fail if its not supported. Fall back to ashmem. Done. Doing it the other way makes a mess. Comment on attachment 184799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review > Source/WebKit/chromium/public/WebSettings.h:97 > + virtual void setDiscardableMemoryEntryLimit(int) = 0; > + virtual void setDiscardableMemoryLimitMB(int) = 0; Nat's right, you don't need this stuff (In reply to comment #52) > (From update of attachment 184799 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review > > > Source/WebKit/chromium/public/WebSettings.h:97 > > + virtual void setDiscardableMemoryEntryLimit(int) = 0; > > + virtual void setDiscardableMemoryLimitMB(int) = 0; > > Nat's right, you don't need this stuff But we dont know how much memory we have allocated and how many files we have allocated, only the cache knows If we put it in DiscardableMemory, then we need to introduce a static variable there to remember the allocated entries and total memory, is that what you guys want? (In reply to comment #53) > (In reply to comment #52) > > (From update of attachment 184799 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review > > > > > Source/WebKit/chromium/public/WebSettings.h:97 > > > + virtual void setDiscardableMemoryEntryLimit(int) = 0; > > > + virtual void setDiscardableMemoryLimitMB(int) = 0; > > > > Nat's right, you don't need this stuff > > But we dont know how much memory we have allocated and how many files we have allocated, only the cache knows What is "we" in this sentence? The ImageDecodingStore knows exactly how much memory it has allocated from different sources at all times. > > If we put it in DiscardableMemory, then we need to introduce a static variable there to remember the allocated entries and total memory, is that what you guys want? The question is which system is responsible for making policy decisions. Do you want to limit how much ashmem is used on platforms that support it? If so, what limits do you want to have and how do you want them enforced? (In reply to comment #54) > (In reply to comment #53) > > (In reply to comment #52) > > > (From update of attachment 184799 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review > > > > > > > Source/WebKit/chromium/public/WebSettings.h:97 > > > > + virtual void setDiscardableMemoryEntryLimit(int) = 0; > > > > + virtual void setDiscardableMemoryLimitMB(int) = 0; > > > > > > Nat's right, you don't need this stuff > > > > But we dont know how much memory we have allocated and how many files we have allocated, only the cache knows > > What is "we" in this sentence? The ImageDecodingStore knows exactly how much memory it has allocated from different sources at all times. Yes, Imagedecodingstore know that, as long as they are not evicted. And don't forget partially decoded images, they are locked and in the cache. So if you have allow infinite number of entries, the cache will be filled with partially decoded images as they are locked and won't get purged > > > > > If we put it in DiscardableMemory, then we need to introduce a static variable there to remember the allocated entries and total memory, is that what you guys want? > > The question is which system is responsible for making policy decisions. Do you want to limit how much ashmem is used on platforms that support it? If so, what limits do you want to have and how do you want them enforced? There is a file descriptor limit of 1024, we currently use 128 in this case. Grace said don't use more than 1G of ashmem memory. (In reply to comment #55) > > The question is which system is responsible for making policy decisions. Do you want to limit how much ashmem is used on platforms that support it? If so, what limits do you want to have and how do you want them enforced? > > There is a file descriptor limit of 1024, we currently use 128 in this case. > Grace said don't use more than 1G of ashmem memory. OK - but are those limits associated with the platform (i.e. global), or with a specific instance of an image decoding store? It sounds like both of these limits are platform limitations, so they should be implemented by the implementation of WebDiscardableMemory, since no single caller of the API can be aware of all of the possibly users of it. (In reply to comment #56) > (In reply to comment #55) > > > The question is which system is responsible for making policy decisions. Do you want to limit how much ashmem is used on platforms that support it? If so, what limits do you want to have and how do you want them enforced? > > > > There is a file descriptor limit of 1024, we currently use 128 in this case. > > Grace said don't use more than 1G of ashmem memory. > > OK - but are those limits associated with the platform (i.e. global), or with a specific instance of an image decoding store? It sounds like both of these limits are platform limitations, so they should be implemented by the implementation of WebDiscardableMemory, since no single caller of the API can be aware of all of the possibly users of it. I think they are platform limitations. Putting it in WebDiscardableMemory seems ok, I will put 2 functions there and have the actual value defined in web_discardable_memory_impl (In reply to comment #57) > (In reply to comment #56) > > (In reply to comment #55) > > > > The question is which system is responsible for making policy decisions. Do you want to limit how much ashmem is used on platforms that support it? If so, what limits do you want to have and how do you want them enforced? > > > > > > There is a file descriptor limit of 1024, we currently use 128 in this case. > > > Grace said don't use more than 1G of ashmem memory. > > > > OK - but are those limits associated with the platform (i.e. global), or with a specific instance of an image decoding store? It sounds like both of these limits are platform limitations, so they should be implemented by the implementation of WebDiscardableMemory, since no single caller of the API can be aware of all of the possibly users of it. > > I think they are platform limitations. > Putting it in WebDiscardableMemory seems ok, I will put 2 functions there and have the actual value defined in web_discardable_memory_impl OK. They might even belong in the base/ implementation, but let's figure that out in the chromium codereview Created attachment 184817 [details]
Patch
removed all the settings thing, and no more limit on discardable cache entry, will use static counters in base::DiscardableMemory to track the memory allocations. Comment on attachment 184817 [details] Patch Attachment 184817 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16123518 Comment on attachment 184817 [details] Patch Attachment 184817 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16117565 Comment on attachment 184817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184817&action=review > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:245 > + if (!cacheEntry->useCount() && (!cacheEntry->isDiscardable() || !m_cacheLimitInBytes)) I don't understand the condition for !m_cacheLimitInBytes. When will it be zero? It seems to be not necessary. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:251 > + removeFromCacheListInternal(heapCacheEntriesToDelete); I think this line is wrong. Created attachment 184824 [details]
Patch
Comment on attachment 184817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184817&action=review >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:245 >> + if (!cacheEntry->useCount() && (!cacheEntry->isDiscardable() || !m_cacheLimitInBytes)) > > I don't understand the condition for !m_cacheLimitInBytes. When will it be zero? It seems to be not necessary. in the dtor we set it to 0. So in that case, we should just clean up everything. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:251 >> + removeFromCacheListInternal(heapCacheEntriesToDelete); > > I think this line is wrong. fixed, no need to split between heap and discardable now Comment on attachment 184824 [details]
Patch
LGTM.
I would like to see this landed soon so I can hack on this code asap to ensure we use discardable memory safely for partially decoded images (This patch will leak for partially decoded images because they stay pinned.)
LGTM Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > + ASSERT(!m_lockedMemory); > + > + if (!m_firstOnLock) { > + ASSERT(m_discardable); > + m_firstOnLock = true; > + m_lockedMemory = m_discardable->data(); > + } else if (m_discardable->lock()) > + m_lockedMemory = m_discardable->data(); this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches? > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:104 > + if (pixelRef && pixelRef->getURI() && !strncmp(pixelRef->getURI(), labelDiscardable, sizeof(labelDiscardable))) SkString has equals() and operator== and several other utilities. use those, don't use strncmp > Source/WebCore/platform/image-decoders/ImageDecoder.h:422 > + if (m_frameBufferCache.isEmpty()) > + m_frameBufferCache.resize(1); > + m_frameBufferCache[0].setMemoryAllocator(allocator); I don't think this scheme will work for any multi-frame format, since frames in the cache with index > 0 won't have our allocator set. It looks busted for gif/ico at least. I guess if the worst case is that these frames won't use our allocator it's OK for now, but it needs a FIXME here at least pointing out the problem. Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 >> + m_lockedMemory = m_discardable->data(); > > this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches? we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock(). >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:104 >> + if (pixelRef && pixelRef->getURI() && !strncmp(pixelRef->getURI(), labelDiscardable, sizeof(labelDiscardable))) > > SkString has equals() and operator== and several other utilities. use those, don't use strncmp will fix >> Source/WebCore/platform/image-decoders/ImageDecoder.h:422 >> + m_frameBufferCache[0].setMemoryAllocator(allocator); > > I don't think this scheme will work for any multi-frame format, since frames in the cache with index > 0 won't have our allocator set. It looks busted for gif/ico at least. I guess if the worst case is that these frames won't use our allocator it's OK for now, but it needs a FIXME here at least pointing out the problem. multiframe won't work for lazy image decoding, so this should not be hit. I will add a FIXME here, Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:46 > + OwnPtr<DiscardablePixelRef> pixelRef = adoptPtr(new DiscardablePixelRef(ctable, adoptPtr(new SkMutex()))); sorry for misleading you earlier, but if DiscardablePixelRef is SkRefCnt'd then it's not a good idea to put it in an OwnPtr after all. This could go in an SkTAutoUnref<> > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:49 > + dst->setPixelRef(pixelRef.leakPtr())->unref(); i'm not sure what the ->unref here is supposed to be balancing. SkBitmap appears to take a reference on the SkPixelRef, but it's storing it in a member so it should hold a ref. is someone/something else ref()ing the SkPixelRef that we need to compensate for? > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51 > + // It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef. i can't figure out what "... a valid pointer to SkPixelRef" could mean. Do you mean a valid pointer to pixels (aka SkBitmap::fPixels set) ? why is it necessary that happens in this call instead of later? Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review >>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 >>> + m_lockedMemory = m_discardable->data(); >> >> this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches? > > we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock(). why not just set m_lockedMemory in allocAndLockDiscardableMemory when the allocateAndLock... succeeds and skip all this? then the m_lockedMemory pointer will be valid from allocation (if it succeeds) and you don't have to worry about initial setup logic here Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:46 >> + OwnPtr<DiscardablePixelRef> pixelRef = adoptPtr(new DiscardablePixelRef(ctable, adoptPtr(new SkMutex()))); > > sorry for misleading you earlier, but if DiscardablePixelRef is SkRefCnt'd then it's not a good idea to put it in an OwnPtr after all. This could go in an SkTAutoUnref<> ok, will change >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:49 >> + dst->setPixelRef(pixelRef.leakPtr())->unref(); > > i'm not sure what the ->unref here is supposed to be balancing. SkBitmap appears to take a reference on the SkPixelRef, but it's storing it in a member so it should hold a ref. is someone/something else ref()ing the SkPixelRef that we need to compensate for? ok, will remove >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51 >> + // It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef. > > i can't figure out what "... a valid pointer to SkPixelRef" could mean. Do you mean a valid pointer to pixels (aka SkBitmap::fPixels set) ? why is it necessary that happens in this call instead of later? Yes, so in skbitmap::lockpixels（）， updatePixelsFromRef() is the call we really we want, so fPixels will be set. This is to match the bahavior of skMallocPixelRef. >>>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 >>>> + m_lockedMemory = m_discardable->data(); >>> >>> this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches? >> >> we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock(). > > why not just set m_lockedMemory in allocAndLockDiscardableMemory when the allocateAndLock... succeeds and skip all this? then the m_lockedMemory pointer will be valid from allocation (if it succeeds) and you don't have to worry about initial setup logic here We can do that, but we still have to call skbitmap->lockPixels() and that will call into this function. Created attachment 184844 [details]
Patch
Hi, James, I update the code and comments, please take another look. For dst->lockPixels(), we have to call it so skbitmap can have correct pixels. Since the memory have to be locked when the first onLockPixels（） are called, we have to ignore the first onLockPixels(). We cannot leave memory unlocked for the first onLockPixels, coz otherwise we will have a situation that the memory got purged before onLockPixels(). Then we have to create ashmem again, and the same thing happens again... Created attachment 184848 [details]
Patch
One small change to remove m_firstOnLock, so when allocAndLockDiscardableMemory() is called, we initialize m_lockedMemory. And when onLockPixels() gets called, we ignore setting m_lockedMemory if it has already been initialized. This should only affect the first onLockPixels as what m_firstOnLock was trying to do. Comment on attachment 184824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review >>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:49 >>> + dst->setPixelRef(pixelRef.leakPtr())->unref(); >> >> i'm not sure what the ->unref here is supposed to be balancing. SkBitmap appears to take a reference on the SkPixelRef, but it's storing it in a member so it should hold a ref. is someone/something else ref()ing the SkPixelRef that we need to compensate for? > > ok, will remove The usual patterns in Skia are: bar->setFoo(new Foo())->unref(); // "new Foo()" leaves refcnt at 1. // setFoo() bumps it to 2. // unref() sets it back to 1, owned by bar. or: { SkAutoTUnref<Foo> foo(new Foo()); // created w/refcnt 1 bar->setFoo(foo); // foo now has refcnt 2 } // foo goes out of scope -> refcnt 1, owned by bar. I prefer the latter, so there are no explicit unref() calls necessary. The former should only be necessary if the object is allocated in the same call and not stored in a local var. (And of course, we should have real smart pointers, but that's another story.) Created attachment 184886 [details]
Patch
added 2 more unit tests there Comment on attachment 184886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184886&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:36 > +const SkString labelDiscardable("discardable"); this looks like it generates a static initializer - can you check if it does and fix it if so? Created attachment 185049 [details]
Patch
The last patch LGTM. Comment on attachment 184886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184886&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:36 >> +const SkString labelDiscardable("discardable"); > > this looks like it generates a static initializer - can you check if it does and fix it if so? Changed this to const char*. And use DEFINE_STATIC_LOCAL in isDiscardable(). The problem is that DiscardablePixelRefAllocator::allocPixelRef is not protected by mutex, but isDiscardable() is protected by mutex in ImageDecodingStore. So using DEFINE_STATIC_LOCAL in isDiscardable() is fine, and using the const char* in allocPixelRef() is also fine. Stephen, James, any ideas? Comment on attachment 185049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review This patch seems ok with nits fixed. r=me > Source/WebCore/ChangeLog:8 > + This change allows using discardable memory in deferred image decoding path. Nit: in deferred -> in the deferred > Source/WebCore/ChangeLog:11 > + Separate caching policy for discardable memory This is not a sentence. > Source/WebCore/ChangeLog:12 > + Discardable memory allocation could fail, fall back to heap allocation in that case. Nit: comma splice: use semicolon instead of comma, or break this into two sentences. > Source/WebCore/ChangeLog:13 > + Separate cache size limit for heap entries, no limit on discardable entries Same here. This is also not a sentence. > Source/WebCore/ChangeLog:18 > + (WebCore::DiscardablePixelRefAllocator::allocPixelRef): You really should have a description of the per-file changes here. > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:64 > +DiscardablePixelRef::DiscardablePixelRef(SkColorTable* ctable, PassOwnPtr<SkMutex> mutex) Kind of weird to wrap Skia stuff in OwnPtrs, but I guess it's ok in this case since it's not refcounted. > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:110 > + if (pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI())) > + return true; > + return false; Could just be return pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI()) > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:101 > + image->bitmap().lockPixels(); > + SkPixelRef* pixelRef = image->bitmap().pixelRef(); > + if (pixelRef->pixels()) { I think you should check image->bitmap().pixels() here. Once you've called lockPixels(), pixels() should be non-zero. You shouldn't have to poke around into the pixelref to find out. > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:116 > + } Not new to this patch, but it seems really unfortunate to leave the pixels locked here, and require the caller to pair up the lock/unlock. IWBN to have some kind of guard class that ensures they're always matched (ignore me if such a thing already exists). Comment on attachment 185049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:110 >> + return false; > > Could just be > > return pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI()) Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:101 >> + if (pixelRef->pixels()) { > > I think you should check image->bitmap().pixels() here. Once you've called lockPixels(), pixels() should be non-zero. You shouldn't have to poke around into the pixelref to find out. If i remember clearly, the reason i used this is because skBitmap::lockpixels() is not threadsafe. So after lockPixels(), I can get a NULL if I call getPixels(). But since all the cache entries are now protected by the mutex, we are safe to use getPixels（） instead of reading pixels from skPixelRef. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:116 >> + } > > Not new to this patch, but it seems really unfortunate to leave the pixels locked here, and require the caller to pair up the lock/unlock. IWBN to have some kind of guard class that ensures they're always matched (ignore me if such a thing already exists). ImageFrameGenerator and LazyDecoingPixelRef are making sure lock and unlock is balanced. Created attachment 185070 [details]
Patch
Created attachment 185081 [details]
Patch
Comment on attachment 185049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review >> Source/WebCore/ChangeLog:8 >> + This change allows using discardable memory in deferred image decoding path. > > Nit: in deferred -> in the deferred Done. >> Source/WebCore/ChangeLog:11 >> + Separate caching policy for discardable memory > > This is not a sentence. This line is covered in the line below, removed. >> Source/WebCore/ChangeLog:12 >> + Discardable memory allocation could fail, fall back to heap allocation in that case. > > Nit: comma splice: use semicolon instead of comma, or break this into two sentences. Done. >> Source/WebCore/ChangeLog:13 >> + Separate cache size limit for heap entries, no limit on discardable entries > > Same here. This is also not a sentence. Done. >> Source/WebCore/ChangeLog:18 >> + (WebCore::DiscardablePixelRefAllocator::allocPixelRef): > > You really should have a description of the per-file changes here. Done. Comment on attachment 185081 [details]
Patch
Will land manually without CQ.
Comment on attachment 185081 [details] Patch Clearing flags on attachment: 185081 Committed r141020: <http://trac.webkit.org/changeset/141020> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 108143 I think this broke the Chromium Mac Debug builds.
[1/60] CXX obj/third_party/webkit/source/wtf/wtf/text/wtf.stringimpl.o
[2/60] OBJCXX obj/media/video/capture/screen/mac/media.desktop_configuration.o
[3/60] OBJCXX obj/media/video/capture/screen/media.screen_capturer_mac.o
[4/60] CXX obj/third_party/webkit/source/webcore/html/webcore_html.htmlmediaelement.o
[5/60] LIBTOOL-STATIC libwtf.a, POSTBUILDS
[6/60] STAMP obj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_prerequisites.actions_depends.stamp
[7/60] STAMP obj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_platform.actions_depends.stamp
[8/60] CXX obj/cc/cc.picture_layer.o
[9/60] CXX obj/cc/cc.picture_layer_impl.o
[10/60] SOLINK libmedia.dylib, POSTBUILDS
[11/60] LIBTOOL-STATIC libwebcore_html.a, POSTBUILDS
[12/60] STAMP obj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore.actions_depends.stamp
[13/60] STAMP obj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_test_support.actions_depends.stamp
[14/60] SOLINK libwebkit.dylib, POSTBUILDS
FAILED: if [ ! -e libwebkit.dylib -o ! -e libwebkit.dylib.TOC ] || otool -l libwebkit.dylib | grep -q LC_REEXPORT_DYLIB ; then ./gyp-mac-tool flock linker.lock clang++ -shared -Wl,-search_paths_first -mmacosx-version-min=10.6 -isysroot /Developer/SDKs/MacOSX10.6.sdk -arch i386 -L. -install_name @rpath/libwebkit.dylib -Wl,-rpath,@loader_path/. -Wl,-rpath,@loader_path/../../.. -o libwebkit.dylib obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.extensions3dchromium.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.graphicscontext3dchromium.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.graphicscontext3dprivate.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webaudiobus.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webdata.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webhttpbody.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webhttploadinfo.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediaconstraints.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamcomponent.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamdescriptor.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamsource.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamsourcesrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webprerender.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcconfiguration.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcicecandidate.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcsessiondescription.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcsessiondescriptionrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcstatsrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcstatsresponse.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcvoidrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webscrollbarthemegeometrynative.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webscrollbarimpl.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.websharedgraphicscontext3d.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webthreadsafedata.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webtransformkeyframe.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webtransformationmatrix.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburl.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlerror.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlresponse.o obj/third_party/webkit/source/webkit/chromium/src/webkit.applicationcachehost.o obj/third_party/webkit/source/webkit/chromium/src/webkit.assertmatchingenums.o obj/third_party/webkit/source/webkit/chromium/src/webkit.associatedurlloader.o obj/third_party/webkit/source/webkit/chromium/src/webkit.asyncfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.asyncfilewriterchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.autofillpopupmenuclient.o obj/third_party/webkit/source/webkit/chromium/src/webkit.backforwardlistchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.batteryclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.datetimechooserimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.chromeclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.colorchooserpopupuicontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.colorchooseruicontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.compositionunderlinevectorbuilder.o obj/third_party/webkit/source/webkit/chromium/src/webkit.contextfeaturesclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.contextmenuclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.databaseobserver.o obj/third_party/webkit/source/webkit/chromium/src/webkit.deliveredintentclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.deviceorientationclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.domutilitiesprivate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.dragclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.dragscrolltimer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.editorclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.eventlistenerwrapper.o obj/third_party/webkit/source/webkit/chromium/src/webkit.externaldatetimechooser.o obj/third_party/webkit/source/webkit/chromium/src/webkit.externalpopupmenu.o obj/third_party/webkit/source/webkit/chromium/src/webkit.findinpagecoordinates.o obj/third_party/webkit/source/webkit/chromium/src/webkit.frameloaderclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.framenetworkingcontextimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.geolocationclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhelperpluginimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbcallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbcursorbackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbdatabasecallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbdatabasebackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbfactorybackendinterface.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbfactorybackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbtransactionbackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbtransactioncallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.inspectorclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.inspectorfrontendclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.linkhighlight.o obj/third_party/webkit/source/webkit/chromium/src/webkit.noncompositedcontenthost.o obj/third_party/webkit/source/webkit/chromium/src/webkit.prerendererclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.webinputeventfactory.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.webscreeninfofactory.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.websubstringutil.o obj/third_party/webkit/source/webkit/chromium/src/webkit.localfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.localizedstrings.o obj/third_party/webkit/source/webkit/chromium/src/webkit.mediaplayerprivatechromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.notificationpresenterimpl.o obj/third_party/webkit/source/webkit/chromium/src/painting/webkit.continuouspainter.o obj/third_party/webkit/source/webkit/chromium/src/painting/webkit.paintaggregator.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pageoverlay.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pageoverlaylist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pagewidgetdelegate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.platformmessageportchannel.o obj/third_party/webkit/source/webkit/chromium/src/webkit.scrollbargroup.o obj/third_party/webkit/source/webkit/chromium/src/webkit.sharedworkerrepository.o obj/third_party/webkit/source/webkit/chromium/src/webkit.speechinputclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.speechrecognitionclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storageareaproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storageinfochromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storagenamespaceproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.textfielddecoratorimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.usermediaclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.validationmessageclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextcheckingcompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextcheckingresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webaccessibilityobject.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webanimationcontrollerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webarraybuffer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webarraybufferview.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webbindings.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webblob.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webblobdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcachedurlrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcolorname.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcommon.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcompositorinputhandlerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcrossoriginpreflightresultcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcursorinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomcustomevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomeventlistener.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomeventlistenerprivate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommessageevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommouseevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommutationevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomstringlist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdatabase.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdatasourceimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdevtoolsagentimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdevtoolsfrontendimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientation.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientationclientmock.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientationcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdocument.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdocumenttype.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdragdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webentities.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfilechoosercompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfilesystemcallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontdescription.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webformcontrolelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webformelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webframeimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationclientmock.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationerror.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationpermissionrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationpermissionrequestmanager.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationposition.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webglyphcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgraphicscontext3d.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhistoryitem.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhittestresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webiconloadingcompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbcallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbcursorimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabasecallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabaseerror.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabaseimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbfactoryimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkey.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkeypath.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkeyrange.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbmetadata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbtransactionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbtransactioncallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webimagedecoder.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webimageskia.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputeventconversion.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintentrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintentserviceinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webkit.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weblabelelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webmediaplayerclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webmediastreamregistry.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnetworkstatenotifier.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnode.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnodecollection.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnodelist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnotification.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weboptionelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpagepopupimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpageserializer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpageserializerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpasswordformdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpasswordformutils.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webperformance.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webplugincontainerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webplugindocument.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpluginloadobserver.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpluginscrollbarimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpopupmenuimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webrange.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webregularexpression.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webruntimefeatures.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscopedmicrotasksuppression.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscopedusergesture.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscriptcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscrollbarthemeclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscrollbarthemepainter.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websearchableformdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websecurityorigin.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websecuritypolicy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webselectelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webserializedscriptvalue.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websettingsimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websharedworkerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websocket.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websocketimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechgrammar.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechinputresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechrecognitionhandle.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechrecognitionresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webstorageeventdispatcherimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webstoragequotacallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websurroundingtext.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextinputinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextrun.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weburlloadtiming.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextfielddecoratorclient.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webusermediarequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webviewbenchmarksupportimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webviewimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerbase.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerrunloop.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerasyncfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerasyncfilewriterchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workercontextproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerfilesystemcallbacksbridge.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerfilewritercallbacksbridge.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.animationtranslationutiltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.canvas2dlayerbridgetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.canvas2dlayermanagertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.chromeclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.clipboardchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.datetimeformattest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.decimaltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.deferredimagedecodertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.dragimagetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.filteroperationstest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.frameloaderclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.graphicslayerchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbabortoncorrupttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbdatabasebackendtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbkeypathtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbleveldbcodingtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imagedecodingstoretest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imageframegeneratortest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imagelayerchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.keyboardtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.kurltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.memoryinfo.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.opaquerecttrackingcontentlayerdelegatetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.opentypeverticaldatatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podarenatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podintervaltreetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podredblacktreetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.paintaggregatortest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.platformcontextskiatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.popupcontainertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.regiontest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.threadsafedatatransporttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.tilingdatatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.treetesthelpers.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webcompositorinputhandlerimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webinputeventconversiontest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webinputeventfactorytestmac.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webmediaplayerclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.websocketdeflatertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.websocketextensiondispatchertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.weburlrequesttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.weburlresponsetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.localemactest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtestingsupport.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webunittests.o libdynamic_annotations.a libgmock.a libwebkit_platform.a libwebcore_platform.a libqcms.a libtess.a libwebcore_test_support.a libtest_support_base.a libwebp_dec.a libleveldatabase.a libwebcore_svg.a libwtf.a libwebp_utils.a libwebcore_dom.a libwebcore_html.a libwebp_enc.a libxslt.a libsqlite3.a libots.a libchrome_zlib.a libbase_static.a libicudata.a libiccjpeg.a libgtest.a libmodp_b64.a libpng.a libwebcore_bindings.a libjpeg_turbo.a libxml2.a libwebp_dsp.a libwebcore_platform_geometry.a libwebcore_rendering.a libharfbuzz-ng.a libwebkit_wtf_support.a libwebcore_remaining.a libv8-i18n.a libyarr.a libv8.dylib libgoogleurl.dylib libcrnspr.dylib libbase.dylib libtranslator_glsl.dylib libgles2_c_lib.dylib libcrnss.dylib libbase_i18n.dylib libicui18n.dylib libicuuc.dylib libskia.dylib -framework Accelerate -framework OpenGL -framework AppKit -lWebKitSystemInterfaceLeopardPrivateExtern -framework QuartzCore -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework Security -framework CoreServices && (export BUILT_PRODUCTS_DIR=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug; export CONFIGURATION=Debug; export DYLIB_INSTALL_NAME_BASE=@rpath; export EXECUTABLE_NAME=libwebkit.dylib; export EXECUTABLE_PATH=libwebkit.dylib; export FULL_PRODUCT_NAME=libwebkit.dylib; export LD_DYLIB_INSTALL_NAME=@rpath/libwebkit.dylib; export MACH_O_TYPE=mh_dylib; export PRODUCT_NAME=webkit; export PRODUCT_TYPE=com.apple.product-type.library.dynamic; export SDKROOT=/Developer/SDKs/MacOSX10.6.sdk; export SRCROOT=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug/../../third_party/WebKit/Source/WebKit/chromium; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug; export TEMP_DIR="${TMPDIR}"; (F=0; cd ../../third_party/WebKit/Source/WebKit/chromium || F=$?; ../../../../../build/mac/strip_from_xcode || F=$?; exit $F); G=$?; ((exit $G) || rm -rf libwebkit.dylib) && exit $G) && { otool -l libwebkit.dylib | grep LC_ID_DYLIB -A 5; nm -gP libwebkit.dylib | cut -f1-2 -d' ' | grep -v U$; true; } > libwebkit.dylib.TOC; else ./gyp-mac-tool flock linker.lock clang++ -shared -Wl,-search_paths_first -mmacosx-version-min=10.6 -isysroot /Developer/SDKs/MacOSX10.6.sdk -arch i386 -L. -install_name @rpath/libwebkit.dylib -Wl,-rpath,@loader_path/. -Wl,-rpath,@loader_path/../../.. -o libwebkit.dylib obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.extensions3dchromium.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.graphicscontext3dchromium.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.graphicscontext3dprivate.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webaudiobus.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webdata.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webhttpbody.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webhttploadinfo.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediaconstraints.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamcomponent.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamdescriptor.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamsource.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webmediastreamsourcesrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webprerender.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcconfiguration.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcicecandidate.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcsessiondescription.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcsessiondescriptionrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcstatsrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcstatsresponse.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webrtcvoidrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webscrollbarthemegeometrynative.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webscrollbarimpl.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.websharedgraphicscontext3d.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webthreadsafedata.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webtransformkeyframe.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.webtransformationmatrix.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburl.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlerror.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlrequest.o obj/third_party/webkit/source/webcore/platform/chromium/support/webkit.weburlresponse.o obj/third_party/webkit/source/webkit/chromium/src/webkit.applicationcachehost.o obj/third_party/webkit/source/webkit/chromium/src/webkit.assertmatchingenums.o obj/third_party/webkit/source/webkit/chromium/src/webkit.associatedurlloader.o obj/third_party/webkit/source/webkit/chromium/src/webkit.asyncfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.asyncfilewriterchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.autofillpopupmenuclient.o obj/third_party/webkit/source/webkit/chromium/src/webkit.backforwardlistchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.batteryclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.datetimechooserimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.chromeclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.colorchooserpopupuicontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.colorchooseruicontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.compositionunderlinevectorbuilder.o obj/third_party/webkit/source/webkit/chromium/src/webkit.contextfeaturesclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.contextmenuclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.databaseobserver.o obj/third_party/webkit/source/webkit/chromium/src/webkit.deliveredintentclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.deviceorientationclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.domutilitiesprivate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.dragclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.dragscrolltimer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.editorclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.eventlistenerwrapper.o obj/third_party/webkit/source/webkit/chromium/src/webkit.externaldatetimechooser.o obj/third_party/webkit/source/webkit/chromium/src/webkit.externalpopupmenu.o obj/third_party/webkit/source/webkit/chromium/src/webkit.findinpagecoordinates.o obj/third_party/webkit/source/webkit/chromium/src/webkit.frameloaderclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.framenetworkingcontextimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.geolocationclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhelperpluginimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbcallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbcursorbackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbdatabasecallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbdatabasebackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbfactorybackendinterface.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbfactorybackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbtransactionbackendproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.idbtransactioncallbacksproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.inspectorclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.inspectorfrontendclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.linkhighlight.o obj/third_party/webkit/source/webkit/chromium/src/webkit.noncompositedcontenthost.o obj/third_party/webkit/source/webkit/chromium/src/webkit.prerendererclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.webinputeventfactory.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.webscreeninfofactory.o obj/third_party/webkit/source/webkit/chromium/src/mac/webkit.websubstringutil.o obj/third_party/webkit/source/webkit/chromium/src/webkit.localfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.localizedstrings.o obj/third_party/webkit/source/webkit/chromium/src/webkit.mediaplayerprivatechromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.notificationpresenterimpl.o obj/third_party/webkit/source/webkit/chromium/src/painting/webkit.continuouspainter.o obj/third_party/webkit/source/webkit/chromium/src/painting/webkit.paintaggregator.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pageoverlay.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pageoverlaylist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.pagewidgetdelegate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.platformmessageportchannel.o obj/third_party/webkit/source/webkit/chromium/src/webkit.scrollbargroup.o obj/third_party/webkit/source/webkit/chromium/src/webkit.sharedworkerrepository.o obj/third_party/webkit/source/webkit/chromium/src/webkit.speechinputclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.speechrecognitionclientproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storageareaproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storageinfochromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.storagenamespaceproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.textfielddecoratorimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.usermediaclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.validationmessageclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextcheckingcompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextcheckingresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webaccessibilityobject.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webanimationcontrollerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webarraybuffer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webarraybufferview.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webbindings.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webblob.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webblobdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcachedurlrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcolorname.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcommon.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcompositorinputhandlerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcrossoriginpreflightresultcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webcursorinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomcustomevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomeventlistener.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomeventlistenerprivate.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommessageevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommouseevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdommutationevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdomstringlist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdatabase.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdatasourceimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdevtoolsagentimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdevtoolsfrontendimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientation.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientationclientmock.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdeviceorientationcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdocument.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdocumenttype.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webdragdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webentities.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfilechoosercompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfilesystemcallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontdescription.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webfontimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webformcontrolelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webformelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webframeimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationclientmock.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationerror.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationpermissionrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationpermissionrequestmanager.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgeolocationposition.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webglyphcache.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webgraphicscontext3d.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhistoryitem.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webhittestresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webiconloadingcompletionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbcallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbcursorimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabasecallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabaseerror.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbdatabaseimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbfactoryimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkey.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkeypath.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbkeyrange.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbmetadata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbtransactionimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webidbtransactioncallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webimagedecoder.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webimageskia.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputevent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webinputeventconversion.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintent.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintentrequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webintentserviceinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webkit.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weblabelelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webmediaplayerclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webmediastreamregistry.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnetworkstatenotifier.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnode.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnodecollection.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnodelist.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webnotification.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weboptionelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpagepopupimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpageserializer.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpageserializerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpasswordformdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpasswordformutils.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webperformance.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webplugincontainerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webplugindocument.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpluginloadobserver.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpluginscrollbarimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webpopupmenuimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webrange.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webregularexpression.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webruntimefeatures.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscopedmicrotasksuppression.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscopedusergesture.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscriptcontroller.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscrollbarthemeclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webscrollbarthemepainter.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websearchableformdata.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websecurityorigin.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websecuritypolicy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webselectelement.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webserializedscriptvalue.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websettingsimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websharedworkerimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websocket.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websocketimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechgrammar.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechinputresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechrecognitionhandle.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webspeechrecognitionresult.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webstorageeventdispatcherimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webstoragequotacallbacksimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.websurroundingtext.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextinputinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextrun.o obj/third_party/webkit/source/webkit/chromium/src/webkit.weburlloadtiming.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtextfielddecoratorclient.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webusermediarequest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webviewbenchmarksupportimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webviewimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerbase.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerclientimpl.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerinfo.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webworkerrunloop.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerasyncfilesystemchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerasyncfilewriterchromium.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workercontextproxy.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerfilesystemcallbacksbridge.o obj/third_party/webkit/source/webkit/chromium/src/webkit.workerfilewritercallbacksbridge.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.animationtranslationutiltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.canvas2dlayerbridgetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.canvas2dlayermanagertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.chromeclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.clipboardchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.datetimeformattest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.decimaltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.deferredimagedecodertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.dragimagetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.filteroperationstest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.frameloaderclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.graphicslayerchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbabortoncorrupttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbdatabasebackendtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbkeypathtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.idbleveldbcodingtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imagedecodingstoretest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imageframegeneratortest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.imagelayerchromiumtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.keyboardtest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.kurltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.memoryinfo.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.opaquerecttrackingcontentlayerdelegatetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.opentypeverticaldatatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podarenatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podintervaltreetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.podredblacktreetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.paintaggregatortest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.platformcontextskiatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.popupcontainertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.regiontest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.threadsafedatatransporttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.tilingdatatest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.treetesthelpers.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webcompositorinputhandlerimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webinputeventconversiontest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webinputeventfactorytestmac.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webmediaplayerclientimpltest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.websocketdeflatertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.websocketextensiondispatchertest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.weburlrequesttest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.weburlresponsetest.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.localemactest.o obj/third_party/webkit/source/webkit/chromium/src/webkit.webtestingsupport.o obj/third_party/webkit/source/webkit/chromium/tests/webkit.webunittests.o libdynamic_annotations.a libgmock.a libwebkit_platform.a libwebcore_platform.a libqcms.a libtess.a libwebcore_test_support.a libtest_support_base.a libwebp_dec.a libleveldatabase.a libwebcore_svg.a libwtf.a libwebp_utils.a libwebcore_dom.a libwebcore_html.a libwebp_enc.a libxslt.a libsqlite3.a libots.a libchrome_zlib.a libbase_static.a libicudata.a libiccjpeg.a libgtest.a libmodp_b64.a libpng.a libwebcore_bindings.a libjpeg_turbo.a libxml2.a libwebp_dsp.a libwebcore_platform_geometry.a libwebcore_rendering.a libharfbuzz-ng.a libwebkit_wtf_support.a libwebcore_remaining.a libv8-i18n.a libyarr.a libv8.dylib libgoogleurl.dylib libcrnspr.dylib libbase.dylib libtranslator_glsl.dylib libgles2_c_lib.dylib libcrnss.dylib libbase_i18n.dylib libicui18n.dylib libicuuc.dylib libskia.dylib -framework Accelerate -framework OpenGL -framework AppKit -lWebKitSystemInterfaceLeopardPrivateExtern -framework QuartzCore -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework Security -framework CoreServices && (export BUILT_PRODUCTS_DIR=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug; export CONFIGURATION=Debug; export DYLIB_INSTALL_NAME_BASE=@rpath; export EXECUTABLE_NAME=libwebkit.dylib; export EXECUTABLE_PATH=libwebkit.dylib; export FULL_PRODUCT_NAME=libwebkit.dylib; export LD_DYLIB_INSTALL_NAME=@rpath/libwebkit.dylib; export MACH_O_TYPE=mh_dylib; export PRODUCT_NAME=webkit; export PRODUCT_TYPE=com.apple.product-type.library.dynamic; export SDKROOT=/Developer/SDKs/MacOSX10.6.sdk; export SRCROOT=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug/../../third_party/WebKit/Source/WebKit/chromium; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/out/Debug; export TEMP_DIR="${TMPDIR}"; (F=0; cd ../../third_party/WebKit/Source/WebKit/chromium || F=$?; ../../../../../build/mac/strip_from_xcode || F=$?; exit $F); G=$?; ((exit $G) || rm -rf libwebkit.dylib) && exit $G) && { otool -l libwebkit.dylib | grep LC_ID_DYLIB -A 5; nm -gP libwebkit.dylib | cut -f1-2 -d' ' | grep -v U$; true; } > libwebkit.dylib.tmp && if ! cmp -s libwebkit.dylib.tmp libwebkit.dylib.TOC; then mv libwebkit.dylib.tmp libwebkit.dylib.TOC ; fi; fi
Undefined symbols:
"__ZN7SkMutexC1Ev", referenced from:
__ZN7WebCore28DiscardablePixelRefAllocator13allocPixelRefEP8SkBitmapP12SkColorTable in libwebcore_platform.a(webcore_platform.discardablepixelref.o)
"__ZN7SkMutexD1Ev", referenced from:
__ZN3WTF14deleteOwnedPtrI7SkMutexEEvPT_ in libwebcore_platform.a(webcore_platform.discardablepixelref.o)
ld: symbol(s) not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
webkit range: http://trac.webkit.org/log/?verbose=on&rev=141020&stop_rev=141017
I wonder if the WebKit/chromium/DEPS didn't get rolled forward? I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. Comment on attachment 185081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185081&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > + if (!m_lockedMemory && m_discardable->lock()) Could m_discardable ever be 0 here? (In reply to comment #96) > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. Comment on attachment 185081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185081&action=review >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 >> + if (!m_lockedMemory && m_discardable->lock()) > > Could m_discardable ever be 0 here? it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. (In reply to comment #99) > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > Could m_discardable ever be 0 here? > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. Will onLockPixels() still be called? (In reply to comment #100) > (In reply to comment #99) > > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > > > Could m_discardable ever be 0 here? > > > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. > > Will onLockPixels() still be called? In DiscardableMemoryAllocator, if allocAndLockDiscardableMemory() returns false, we will fall back to the heap allocator. So a skMallocPixelRef will be created instead of this one. And onLockPixels() will be called on the SkMallocPixelRef. Was this failure caused by this patch too? http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fvirtual%2Fdeferred%2Ffast%2Fimages%2Ficon-0colors.html (In reply to comment #102) > Was this failure caused by this patch too? > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fvirtual%2Fdeferred%2Ffast%2Fimages%2Ficon-0colors.html Looks likely. Lazy decoding should only work for images with 1 frame. I am not sure why this test image with 2 frames need to use lazy decoder. Alpha, any idea why this test need to decode a 2 frame image? (In reply to comment #103) > (In reply to comment #102) > > Was this failure caused by this patch too? > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fvirtual%2Fdeferred%2Ffast%2Fimages%2Ficon-0colors.html > > Looks likely. Lazy decoding should only work for images with 1 frame. I am not sure why this test image with 2 frames need to use lazy decoder. > Alpha, any idea why this test need to decode a 2 frame image? Hm... I think we hit the corner case of multi-frame ICO images. We should come up with a quick fix for this to work properly for images with more than 1 frame. (In reply to comment #98) > (In reply to comment #96) > > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. > > We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. > The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. (In reply to comment #102) > Was this failure caused by this patch too? > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fvirtual%2Fdeferred%2Ffast%2Fimages%2Ficon-0colors.html Keishi, can you file a bug and mark this as debug crash? We'll come up with a fix today. (In reply to comment #105) > (In reply to comment #98) > > (In reply to comment #96) > > > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > > > > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. > > > > We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. > > The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. > > (In reply to comment #102) > > Was this failure caused by this patch too? > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=platform%2Fchromium%2Fvirtual%2Fdeferred%2Ffast%2Fimages%2Ficon-0colors.html > > Keishi, can you file a bug and mark this as debug crash? We'll come up with a fix today. Not today but I mean this week. (In reply to comment #101) > (In reply to comment #100) > > (In reply to comment #99) > > > > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > > > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > > > > > Could m_discardable ever be 0 here? > > > > > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. > > > > Will onLockPixels() still be called? > > In DiscardableMemoryAllocator, if allocAndLockDiscardableMemory() returns false, we will fall back to the heap allocator. So a skMallocPixelRef will be created instead of this one. And onLockPixels() will be called on the SkMallocPixelRef. Right, add an ASSERT(m_discardable) then. (In reply to comment #107) > (In reply to comment #101) > > (In reply to comment #100) > > > (In reply to comment #99) > > > > > > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > > > > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > > > > > > > Could m_discardable ever be 0 here? > > > > > > > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. > > > > > > Will onLockPixels() still be called? > > > > In DiscardableMemoryAllocator, if allocAndLockDiscardableMemory() returns false, we will fall back to the heap allocator. So a skMallocPixelRef will be created instead of this one. And onLockPixels() will be called on the SkMallocPixelRef. > > Right, add an ASSERT(m_discardable) then. I think adding an ASSERT() here will not provide any benefit. If m_discardable is NULL, we crash anyway in the next statement. (In reply to comment #108) > (In reply to comment #107) > > (In reply to comment #101) > > > (In reply to comment #100) > > > > (In reply to comment #99) > > > > > > > > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > > > > > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > > > > > > > > > Could m_discardable ever be 0 here? > > > > > > > > > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. > > > > > > > > Will onLockPixels() still be called? > > > > > > In DiscardableMemoryAllocator, if allocAndLockDiscardableMemory() returns false, we will fall back to the heap allocator. So a skMallocPixelRef will be created instead of this one. And onLockPixels() will be called on the SkMallocPixelRef. > > > > Right, add an ASSERT(m_discardable) then. > > I think adding an ASSERT() here will not provide any benefit. If m_discardable is NULL, we crash anyway in the next statement. That response verges on tautology. Please add the ASSERT. (In reply to comment #109) > (In reply to comment #108) > > (In reply to comment #107) > > > (In reply to comment #101) > > > > (In reply to comment #100) > > > > > (In reply to comment #99) > > > > > > > > > > > >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89 > > > > > > >> + if (!m_lockedMemory && m_discardable->lock()) > > > > > > > > > > > > > > Could m_discardable ever be 0 here? > > > > > > > > > > > > it shouldn't. If allocAndLockDiscardableMemory failed, we should not use the DiscardablePixelRef anymore. > > > > > > > > > > Will onLockPixels() still be called? > > > > > > > > In DiscardableMemoryAllocator, if allocAndLockDiscardableMemory() returns false, we will fall back to the heap allocator. So a skMallocPixelRef will be created instead of this one. And onLockPixels() will be called on the SkMallocPixelRef. > > > > > > Right, add an ASSERT(m_discardable) then. > > > > I think adding an ASSERT() here will not provide any benefit. If m_discardable is NULL, we crash anyway in the next statement. > > That response verges on tautology. Please add the ASSERT. ok, will do that in a separate patch I agree that an ASSERT() is silly and not needed here. Thank you. There should also be a way to test the allocAndLockDiscardableMemory failing case, such as with your mock unit test, no? (In reply to comment #98) > (In reply to comment #96) > > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. > > We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. > The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. I don't think 2 DiscardablePixelRef locked on the same mutex will lead to deadlock, it's more like LazyDecodingPixelRef and DiscardablePixelRef locked the same mutex that can be a problem. I think we should fix this quickly and roll in a new Skia with SK_API for SkMutex. (In reply to comment #113) > (In reply to comment #98) > > (In reply to comment #96) > > > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > > > > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. > > > > We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. > > The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. > > I don't think 2 DiscardablePixelRef locked on the same mutex will lead to deadlock, it's more like LazyDecodingPixelRef and DiscardablePixelRef locked the same mutex that can be a problem. I think we should fix this quickly and roll in a new Skia with SK_API for SkMutex. Yes, you are right. Since cache entries are protected by a mutex in ImageDecodingStore, no 2 DiscardablePixelRef will call lockPixels/unlock pixels at the same. However, as long as there are other Skpixelrefs in the same process, this could lead to deadlock. Currently a process cannot use more than 32 skPixelRef if it is multithreaded, otherwise it will have to call setPrelocked or provide its own mutex. (In reply to comment #114) > (In reply to comment #113) > > (In reply to comment #98) > > > (In reply to comment #96) > > > > I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033 > > > > > > > > The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API. > > > > > > We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. > > > The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks. > > > > I don't think 2 DiscardablePixelRef locked on the same mutex will lead to deadlock, it's more like LazyDecodingPixelRef and DiscardablePixelRef locked the same mutex that can be a problem. I think we should fix this quickly and roll in a new Skia with SK_API for SkMutex. > > Yes, you are right. Since cache entries are protected by a mutex in ImageDecodingStore, no 2 DiscardablePixelRef will call lockPixels/unlock pixels at the same. > However, as long as there are other Skpixelrefs in the same process, this could lead to deadlock. > Currently a process cannot use more than 32 skPixelRef if it is multithreaded, otherwise it will have to call setPrelocked or provide its own mutex. well, actually we just need 2 SkPixelref to cause a deadlock. Just keep creating one and deleting it, until the 2 skpixelref starts to share the same mutex. Essentially entirely Chromium port change, thus now dead with Chromium having forked. |