Bug 106842 – adding support for DiscardablePixelRef for caching lazily decoded images

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Min Qin]

adding support for DiscardablePixelRef for caching lazily decoded images

[Min Qin]

Created attachment 182657 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hin-Chung Lam]

Thanks for the patch. I'll look.

[WebKit Review Bot]

Comment on attachment 182657 [details]
Patch

Attachment 182657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15884365

[Nat Duca]

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 182657 [details]
Patch

Attachment 182657 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15872680

[Min Qin]

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

[Min Qin]

Created attachment 183206 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183206 [details]
Patch

Attachment 183206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15906980

[Peter Beverloo (cr-android ews)]

Comment on attachment 183206 [details]
Patch

Attachment 183206 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15907912

[Min Qin]

Created attachment 183223 [details]
Patch

[Min Qin]

Added the chromium CL as reference in the changeLog.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 183223 [details]
Patch

Attachment 183223 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15944061

[James Robinson]

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 183223 [details]
Patch

Attachment 183223 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15941163

[Nat Duca]

carving off WebDiscardable to https://bugs.webkit.org/show_bug.cgi?id=107183

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

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.

[Stephen White]

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?

[Stephen White]

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.

[Nat Duca]

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.

[Hin-Chung Lam]

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

[Stephen White]

Makes sense.  Thanks for the explanation(s)!

[Min Qin]

Created attachment 183528 [details]
Patch

[Min Qin]

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

[Min Qin]

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

[WebKit Review Bot]

Comment on attachment 183528 [details]
Patch

Attachment 183528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15949465

[James Robinson]

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.

[Peter Beverloo (cr-android ews)]

Comment on attachment 183528 [details]
Patch

Attachment 183528 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15939810

[Min Qin]

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.

[James Robinson]

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

[Min Qin]

Created attachment 184061 [details]
Patch

[Min Qin]

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 ^^

[Hin-Chung Lam]

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?

[Hin-Chung Lam]

By the way I think your patch is mixed with the patch to fix race condition.

[Min Qin]

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

[Min Qin]

Created attachment 184272 [details]
Patch

[James Robinson]

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() ?

[Min Qin]

Created attachment 184379 [details]
Patch

[Min Qin]

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

[Min Qin]

Created attachment 184554 [details]
Patch

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

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.

[Min Qin]

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

[Min Qin]

Created attachment 184799 [details]
Patch

[Min Qin]

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

[Nat Duca]

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.

[James Robinson]

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

[Min Qin]

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

[James Robinson]

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

[Min Qin]

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

[James Robinson]

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

[Min Qin]

(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

[James Robinson]

(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

[Min Qin]

Created attachment 184817 [details]
Patch

[Min Qin]

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.

[WebKit Review Bot]

Comment on attachment 184817 [details]
Patch

Attachment 184817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16123518

[Peter Beverloo (cr-android ews)]

Comment on attachment 184817 [details]
Patch

Attachment 184817 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16117565

[Hin-Chung Lam]

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.

[Min Qin]

Created attachment 184824 [details]
Patch

[Min Qin]

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

[Hin-Chung Lam]

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

[Nat Duca]

LGTM

[James Robinson]

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.

[Min Qin]

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,

[James Robinson]

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?

[James Robinson]

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

[Min Qin]

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.

[Min Qin]

Created attachment 184844 [details]
Patch

[Min Qin]

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

[Min Qin]

Created attachment 184848 [details]
Patch

[Min Qin]

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.

[Stephen White]

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

[Min Qin]

Created attachment 184886 [details]
Patch

[Min Qin]

added 2 more unit tests there

[James Robinson]

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?

[Min Qin]

Created attachment 185049 [details]
Patch

[Hin-Chung Lam]

The last patch LGTM.

[Min Qin]

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.

[Min Qin]

Stephen, James, any ideas?

[Stephen White]

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

[Min Qin]

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.

[Min Qin]

Created attachment 185070 [details]
Patch

[Min Qin]

Created attachment 185081 [details]
Patch

[Min Qin]

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.

[Xianzhu Wang]

Comment on attachment 185081 [details]
Patch

Will land manually without CQ.

[Xianzhu Wang]

Comment on attachment 185081 [details]
Patch

Clearing flags on attachment: 185081

Committed r141020: <http://trac.webkit.org/changeset/141020>

[Xianzhu Wang]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 108143

[Keishi Hattori]

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

[Nat Duca]

I wonder if the WebKit/chromium/DEPS didn't get rolled forward?

[Hin-Chung Lam]

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.

[noel gordon]

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?

[Min Qin]

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

[Min Qin]

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.

[noel gordon]

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

[Min Qin]

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

[Keishi Hattori]

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

[Min Qin]

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

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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

[noel gordon]

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

[Min Qin]

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

[noel gordon]

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

[Min Qin]

(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

[James Robinson]

I agree that an ASSERT() is silly and not needed here.

[noel gordon]

Thank you.  There should also be a way to test the allocAndLockDiscardableMemory failing case, such as with your mock unit test, no?

[Hin-Chung Lam]

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

[Min Qin]

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

[Min Qin]

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

[Sam Sneddon [:gsnedders]]

Essentially entirely Chromium port change, thus now dead with Chromium having forked.