Bug 106842 - adding support for DiscardablePixelRef for caching lazily decoded images
Summary: adding support for DiscardablePixelRef for caching lazily decoded images
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 108143
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-14 16:28 PST by Min Qin
Modified: 2013-01-28 22:11 PST (History)
16 users (show)

See Also:


Attachments
Patch (15.63 KB, patch)
2013-01-14 16:29 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.53 KB, patch)
2013-01-17 10:03 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.60 KB, patch)
2013-01-17 11:10 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (23.46 KB, patch)
2013-01-18 12:37 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (27.73 KB, patch)
2013-01-22 15:36 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (36.20 KB, patch)
2013-01-23 11:49 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (36.25 KB, patch)
2013-01-23 19:13 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (36.23 KB, patch)
2013-01-24 12:30 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (38.01 KB, patch)
2013-01-25 13:11 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.70 KB, patch)
2013-01-25 15:32 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.58 KB, patch)
2013-01-25 15:48 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.60 KB, patch)
2013-01-25 17:30 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (28.49 KB, patch)
2013-01-25 18:24 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (32.44 KB, patch)
2013-01-26 14:38 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (32.66 KB, patch)
2013-01-28 13:29 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (32.56 KB, patch)
2013-01-28 14:50 PST, Min Qin
wangxianzhu: commit-queue-
Details | Formatted Diff | Diff
Patch (33.22 KB, patch)
2013-01-28 15:15 PST, Min Qin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Min Qin 2013-01-14 16:28:00 PST
adding support for DiscardablePixelRef for caching lazily decoded images
Comment 1 Min Qin 2013-01-14 16:29:58 PST
Created attachment 182657 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-14 16:31:41 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Hin-Chung Lam 2013-01-14 16:34:47 PST
Thanks for the patch. I'll look.
Comment 4 WebKit Review Bot 2013-01-14 18:03:17 PST
Comment on attachment 182657 [details]
Patch

Attachment 182657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15884365
Comment 5 Nat Duca 2013-01-14 18:12:32 PST
Comment on attachment 182657 [details]
Patch

Isn't there a way to remove all these modes and stuff by
0) always delegating allocator up to skia, e.g. instead of skia::DiscardableMemoryAllocator, use skia::MemoryAllocator
1) basically making the cache size a webkit preference that we set up in the embedder, and
2) making the lazy pixel ref have a mode saying "are you purged"? eg asume all lazy refs can be purged/lost then just deal with it? The thing is, other than the static cast here, and the size set differently, there's nothign much special webkit side about this...Isn't there a way to remove all these modes and stuff by
0) always delegating allocator up to skia, e.g. instead of skia::DiscardableMemoryAllocator, use skia::MemoryAllocator
1) basically making the cache size a webkit preference that we set up in the embedder, and
2) making the lazy pixel ref have a mode saying "are you purged"? eg asume all lazy refs can be purged/lost then just deal with it? The thing is, other than the static cast here, and the size set differently, there's nothign much special webkit side about this...
Comment 6 Peter Beverloo (cr-android ews) 2013-01-14 18:40:21 PST
Comment on attachment 182657 [details]
Patch

Attachment 182657 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15872680
Comment 7 Min Qin 2013-01-15 09:22:16 PST
(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.
Comment 8 Min Qin 2013-01-17 10:03:52 PST
Created attachment 183206 [details]
Patch
Comment 9 WebKit Review Bot 2013-01-17 10:55:37 PST
Comment on attachment 183206 [details]
Patch

Attachment 183206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15906980
Comment 10 Peter Beverloo (cr-android ews) 2013-01-17 11:02:31 PST
Comment on attachment 183206 [details]
Patch

Attachment 183206 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15907912
Comment 11 Min Qin 2013-01-17 11:10:51 PST
Created attachment 183223 [details]
Patch
Comment 12 Min Qin 2013-01-17 11:13:06 PST
Added the chromium CL as reference in the changeLog.
Comment 13 Eric Seidel (no email) 2013-01-17 11:32:53 PST
I feel like other platforms have discardable buffer w/o webcore having to know about it.  It's not imediately clear to me why this is needed in WebCore.
Comment 14 WebKit Review Bot 2013-01-17 11:41:47 PST
Comment on attachment 183223 [details]
Patch

Attachment 183223 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15944061
Comment 15 James Robinson 2013-01-17 12:38:48 PST
(In reply to comment #13)
> I feel like other platforms have discardable buffer w/o webcore having to know about it.  It's not imediately clear to me why this is needed in WebCore.

We want to use discardable memory to back the memory that the WebCore image decoders use, which means that at some level this needs to be hooked into WebCore.
Comment 16 Peter Beverloo (cr-android ews) 2013-01-17 12:44:53 PST
Comment on attachment 183223 [details]
Patch

Attachment 183223 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15941163
Comment 17 Nat Duca 2013-01-17 15:00:56 PST
carving off WebDiscardable to https://bugs.webkit.org/show_bug.cgi?id=107183
Comment 18 Hin-Chung Lam 2013-01-17 15:21:26 PST
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.
Comment 19 Hin-Chung Lam 2013-01-17 15:22:36 PST
Sorry for the delay. I looked through the code and the approach seems right to me. I would like see another pass with DiscardablePixelRef taken out as a separate patch. Thanks.
Comment 20 Hin-Chung Lam 2013-01-17 16:02:54 PST
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.
Comment 21 Stephen White 2013-01-17 21:36:17 PST
One thing I'm not clear on is why we need a discardable pixelref, as well as one that does deferred decoding.  Shouldn't the deferred decoding one simply allocate discardable memory onLockPixels()?  (One PixelRef to rule them all..)  Or is this for cacheing resized subsets?
Comment 22 Stephen White 2013-01-17 21:53:28 PST
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.
Comment 23 Nat Duca 2013-01-17 21:58:43 PST
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.
Comment 24 Hin-Chung Lam 2013-01-17 23:49:21 PST
(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)
Comment 25 Stephen White 2013-01-18 06:28:37 PST
Makes sense.  Thanks for the explanation(s)!
Comment 26 Min Qin 2013-01-18 12:37:29 PST
Created attachment 183528 [details]
Patch
Comment 27 Min Qin 2013-01-18 12:46:37 PST
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
Comment 28 Min Qin 2013-01-18 12:50:46 PST
PTAL.

This change now depends on https://bugs.webkit.org/show_bug.cgi?id=107183 as WebDiscardableMemory code has been moved there. We can wait until that change got landed and then land this one
Comment 29 WebKit Review Bot 2013-01-18 13:53:45 PST
Comment on attachment 183528 [details]
Patch

Attachment 183528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15949465
Comment 30 James Robinson 2013-01-18 14:18:10 PST
Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ?  Can we use that directly?  If not, can we look at how it's implemented and learn anything?  For instance, it seems to always round the allocation size up to the next page size.
Comment 31 Peter Beverloo (cr-android ews) 2013-01-18 15:02:34 PST
Comment on attachment 183528 [details]
Patch

Attachment 183528 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15939810
Comment 32 Min Qin 2013-01-18 15:30:18 PST
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.
Comment 33 James Robinson 2013-01-22 15:29:14 PST
(In reply to comment #30)
> Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ?  Can we use that directly?  If not, can we look at how it's implemented and learn anything?  For instance, it seems to always round the allocation size up to the next page size.

I'd really like an answer to ^^
Comment 34 Min Qin 2013-01-22 15:36:55 PST
Created attachment 184061 [details]
Patch
Comment 35 Min Qin 2013-01-22 16:01:38 PST
I don't think we can easily use that class.
1. It is not threadsafe. It uses a mutex ring which is not threadsafe.
2. It uses SkImageDecoder, not sure how much effort we will have to maintain that later on.
3. For a single image, we can have multiple cache entries(different resolutions, rects.) And we have two PixelRefs. One placeholder(LazyDecodingPixelRef), one stores the pixel(DiscardablePixelRef). Decode() is called on a single placeholder, while storage is on different DiscardablePixelRefs. So there is a 1:M relationship here.

I think we can add the allocation size limit here, but I am not sure whether we should have it in this patch. Maybe a follow up one.

(In reply to comment #33)
> (In reply to comment #30)
> > Has anyone looked at http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.h / http://code.google.com/p/skia/source/browse/trunk/src/ports/SkImageRef_ashmem.cpp ?  Can we use that directly?  If not, can we look at how it's implemented and learn anything?  For instance, it seems to always round the allocation size up to the next page size.
> 
> I'd really like an answer to ^^
Comment 36 Hin-Chung Lam 2013-01-23 10:50:01 PST
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?
Comment 37 Hin-Chung Lam 2013-01-23 10:54:52 PST
By the way I think your patch is mixed with the patch to fix race condition.
Comment 38 Min Qin 2013-01-23 11:46:42 PST
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().
Comment 39 Min Qin 2013-01-23 11:49:17 PST
Created attachment 184272 [details]
Patch
Comment 40 James Robinson 2013-01-23 17:52:34 PST
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() ?
Comment 41 Min Qin 2013-01-23 19:13:15 PST
Created attachment 184379 [details]
Patch
Comment 42 Min Qin 2013-01-23 19:15:56 PST
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
Comment 43 Min Qin 2013-01-24 12:30:17 PST
Created attachment 184554 [details]
Patch
Comment 44 Hin-Chung Lam 2013-01-24 12:35:51 PST
(In reply to comment #38)
> (From update of attachment 184061 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184061&action=review
> 
> Uploading a new patch to include recent changes to WebDiscardableMemory and the fix for race conditions. PTAL
> 
> >> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:42
> >> +    // unlockPixels() will be called when the image decoder goes away.
> > 
> > Does it mean that lock to DiscardablePixelRef is held for the entire lifetime of ImageDecoder?
> 
> Yes. In ImageDecodingStore::insertAndLockCache(), when the decoder is going away, the SkBitmap associated with the imageDecoder will also gone. That's when the unlockPixels() gets called.
> Since ImageFrameGenerator::decode() assigned that SkBitmap to another Skbitmap(fullSizeBitmap), so the fullSizeBitmap will have its own count on lock/unlockpixels().

I'm still confused with this statement.

SkBitmap doesn't call SkPixelRef::lockPixels() on its own, it is called only when needed isn't it? Let's have  VC today, I don't want to block you for too long.
Comment 45 Hin-Chung Lam 2013-01-25 11:25:49 PST
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.
Comment 46 Hin-Chung Lam 2013-01-25 11:30:02 PST
We discussed thoroughly offline about this patch.

We settled on the limitation of this patch, that is:

1. Fully decoded cache entries is unlocked and memory unpinned.
2. Partially decoded cache entries is not unlocked and memory stay pinned.

Because there is a separate memory limit and cache count limit, the memory usage by partially decoded cache entries will not go unbounded. It is safe to accept this patch with above limitations.

We agreed to work on a second iteration to complete the part for partially decoded cache entries.
Comment 47 Hin-Chung Lam 2013-01-25 11:52:45 PST
Comment on attachment 184554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184554&action=review

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:113
>> +        if (!pixelRef->pixels()) {
> 
> 1. Can we move this check before line 103 such that we only increment use count if pixelRef->pixels() return true?
> 
> 2. Please also run removeCacheEntryInternal(cacheEntry) outside of the lock. You can have another success variable and return outside of the block.

Sorry what I mean for 2 is to delete the CacheEntry outside the lock. I would merge removeCacheEntryInternal here and put the Vector<...> above the mutex.
Comment 48 Min Qin 2013-01-25 13:11:17 PST
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
Comment 49 Min Qin 2013-01-25 13:11:38 PST
Created attachment 184799 [details]
Patch
Comment 50 Min Qin 2013-01-25 13:13:53 PST
James, Stephen, would you please take a look at this change?

BTW, there is a chromium change based on this: 
https://codereview.chromium.org/12077002/
James, would you please also take a look on that?

Thanks
Comment 51 Nat Duca 2013-01-25 13:26:18 PST
Comment on attachment 184799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184799&action=review

> Source/WebKit/chromium/ChangeLog:8
> +        Adding DiscardableMemory preferences to WebSettings and new tests for ImageDecodingStore

Dont have a preference for this. Its either supported by the platform, or its not. Dont probe for support. Just try creating the discardable. It will fail if its not supported. Fall back to ashmem. Done.  Doing it the other way makes a mess.
Comment 52 James Robinson 2013-01-25 13:29:57 PST
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
Comment 53 Min Qin 2013-01-25 13:32:08 PST
(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?
Comment 54 James Robinson 2013-01-25 13:35:00 PST
(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?
Comment 55 Min Qin 2013-01-25 13:43:22 PST
(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.
Comment 56 James Robinson 2013-01-25 13:54:09 PST
(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.
Comment 57 Min Qin 2013-01-25 14:00:21 PST
(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
Comment 58 James Robinson 2013-01-25 14:05:35 PST
(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
Comment 59 Min Qin 2013-01-25 15:32:51 PST
Created attachment 184817 [details]
Patch
Comment 60 Min Qin 2013-01-25 15:34:47 PST
removed all the settings thing, and no more limit on discardable cache entry, will use static counters in base::DiscardableMemory to track the memory allocations.
Comment 61 WebKit Review Bot 2013-01-25 15:39:50 PST
Comment on attachment 184817 [details]
Patch

Attachment 184817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16123518
Comment 62 Peter Beverloo (cr-android ews) 2013-01-25 15:44:03 PST
Comment on attachment 184817 [details]
Patch

Attachment 184817 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16117565
Comment 63 Hin-Chung Lam 2013-01-25 15:46:20 PST
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.
Comment 64 Min Qin 2013-01-25 15:48:00 PST
Created attachment 184824 [details]
Patch
Comment 65 Min Qin 2013-01-25 15:52:35 PST
Comment on attachment 184817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184817&action=review

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:245
>> +            if (!cacheEntry->useCount() && (!cacheEntry->isDiscardable() || !m_cacheLimitInBytes))
> 
> I don't understand the condition for !m_cacheLimitInBytes. When will it be zero? It seems to be not necessary.

in the dtor we set it to 0. So in that case, we should just clean up everything.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:251
>> +        removeFromCacheListInternal(heapCacheEntriesToDelete);
> 
> I think this line is wrong.

fixed, no need to split between heap and discardable now
Comment 66 Hin-Chung Lam 2013-01-25 16:02:44 PST
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.)
Comment 67 Nat Duca 2013-01-25 16:05:18 PST
LGTM
Comment 68 James Robinson 2013-01-25 16:38:55 PST
Comment on attachment 184824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89
> +    ASSERT(!m_lockedMemory);
> +
> +    if (!m_firstOnLock) {
> +        ASSERT(m_discardable);
> +        m_firstOnLock = true;
> +        m_lockedMemory = m_discardable->data();
> +    } else if (m_discardable->lock())
> +        m_lockedMemory = m_discardable->data();

this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches?

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:104
> +    if (pixelRef && pixelRef->getURI() && !strncmp(pixelRef->getURI(), labelDiscardable, sizeof(labelDiscardable)))

SkString has equals() and operator== and several other utilities. use those, don't use strncmp

> Source/WebCore/platform/image-decoders/ImageDecoder.h:422
> +            if (m_frameBufferCache.isEmpty())
> +                m_frameBufferCache.resize(1);
> +            m_frameBufferCache[0].setMemoryAllocator(allocator);

I don't think this scheme will work for any multi-frame format, since frames in the cache with index > 0 won't have our allocator set.  It looks busted for gif/ico at least.  I guess if the worst case is that these frames won't use our allocator it's OK for now, but it needs a FIXME here at least pointing out the problem.
Comment 69 Min Qin 2013-01-25 16:43:39 PST
Comment on attachment 184824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89
>> +        m_lockedMemory = m_discardable->data();
> 
> this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches?

we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock().

>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:104
>> +    if (pixelRef && pixelRef->getURI() && !strncmp(pixelRef->getURI(), labelDiscardable, sizeof(labelDiscardable)))
> 
> SkString has equals() and operator== and several other utilities. use those, don't use strncmp

will fix

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:422
>> +            m_frameBufferCache[0].setMemoryAllocator(allocator);
> 
> I don't think this scheme will work for any multi-frame format, since frames in the cache with index > 0 won't have our allocator set.  It looks busted for gif/ico at least.  I guess if the worst case is that these frames won't use our allocator it's OK for now, but it needs a FIXME here at least pointing out the problem.

multiframe won't work for lazy image decoding, so this should not be hit. I will add a FIXME here,
Comment 70 James Robinson 2013-01-25 16:46:44 PST
Comment on attachment 184824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:46
> +    OwnPtr<DiscardablePixelRef> pixelRef = adoptPtr(new DiscardablePixelRef(ctable, adoptPtr(new SkMutex())));

sorry for misleading you earlier, but if DiscardablePixelRef is SkRefCnt'd  then it's not a good idea to put it in an OwnPtr after all. This could go in an SkTAutoUnref<>

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:49
> +        dst->setPixelRef(pixelRef.leakPtr())->unref();

i'm not sure what the ->unref here is supposed to be balancing. SkBitmap appears to take a reference on the SkPixelRef, but it's storing it in a member so it should hold a ref.  is someone/something else ref()ing the SkPixelRef that we need to compensate for?

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:51
> +        // It is necessary to lock this SkBitmap to have a valid pointer to SkPixelRef.

i can't figure out what "... a valid pointer to SkPixelRef" could mean. Do you mean a valid pointer to pixels (aka SkBitmap::fPixels set) ? why is it necessary that happens in this call instead of later?
Comment 71 James Robinson 2013-01-25 16:49:39 PST
Comment on attachment 184824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review

>>> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89
>>> +        m_lockedMemory = m_discardable->data();
>> 
>> this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches?
> 
> we cannot lock the memory twice, since the memory is initially locked after allocation, the first onLockPixels() should not lock the memory again. So m_firstOnLock just skips it, while in other cases we call lock().

why not just set m_lockedMemory in allocAndLockDiscardableMemory when the allocateAndLock... succeeds and skip all this? then the m_lockedMemory pointer will be valid from allocation (if it succeeds) and you don't have to worry about initial setup logic here
Comment 72 Min Qin 2013-01-25 17:01:36 PST
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.
Comment 73 Min Qin 2013-01-25 17:30:57 PST
Created attachment 184844 [details]
Patch
Comment 74 Min Qin 2013-01-25 17:36:22 PST
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...
Comment 75 Min Qin 2013-01-25 18:24:22 PST
Created attachment 184848 [details]
Patch
Comment 76 Min Qin 2013-01-25 18:28:07 PST
One small change to remove m_firstOnLock, so when allocAndLockDiscardableMemory() is called, we initialize m_lockedMemory.

And when onLockPixels() gets called, we ignore setting m_lockedMemory if it has already been initialized. This should only affect the first onLockPixels as what m_firstOnLock was trying to do.
Comment 77 Stephen White 2013-01-26 12:48:48 PST
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.)
Comment 78 Min Qin 2013-01-26 14:38:27 PST
Created attachment 184886 [details]
Patch
Comment 79 Min Qin 2013-01-26 14:39:35 PST
added 2 more unit tests there
Comment 80 James Robinson 2013-01-28 11:42:18 PST
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?
Comment 81 Min Qin 2013-01-28 13:29:05 PST
Created attachment 185049 [details]
Patch
Comment 82 Hin-Chung Lam 2013-01-28 13:34:29 PST
The last patch LGTM.
Comment 83 Min Qin 2013-01-28 13:35:19 PST
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.
Comment 84 Min Qin 2013-01-28 13:37:46 PST
Stephen, James, any ideas?
Comment 85 Stephen White 2013-01-28 14:17:51 PST
Comment on attachment 185049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review

This patch seems ok with nits fixed.  r=me

> Source/WebCore/ChangeLog:8
> +        This change allows using discardable memory in deferred image decoding path.

Nit:  in deferred -> in the deferred

> Source/WebCore/ChangeLog:11
> +        Separate caching policy for discardable memory

This is not a sentence.

> Source/WebCore/ChangeLog:12
> +        Discardable memory allocation could fail, fall back to heap allocation in that case.

Nit:  comma splice:  use semicolon instead of comma, or break this into two sentences.

> Source/WebCore/ChangeLog:13
> +        Separate cache size limit for heap entries, no limit on discardable entries

Same here.  This is also not a sentence.

> Source/WebCore/ChangeLog:18
> +        (WebCore::DiscardablePixelRefAllocator::allocPixelRef):

You really should have a description of the per-file changes here.

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:64
> +DiscardablePixelRef::DiscardablePixelRef(SkColorTable* ctable, PassOwnPtr<SkMutex> mutex)

Kind of weird to wrap Skia stuff in OwnPtrs, but I guess it's ok in this case since it's not refcounted.

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:110
> +    if (pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI()))
> +        return true;
> +    return false;

Could just be

return pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI())

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:101
> +        image->bitmap().lockPixels();
> +        SkPixelRef* pixelRef = image->bitmap().pixelRef();
> +        if (pixelRef->pixels()) {

I think you should check image->bitmap().pixels() here.  Once you've called lockPixels(), pixels() should be non-zero.  You shouldn't have to poke around into the pixelref to find out.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:116
> +        }

Not new to this patch, but it seems really unfortunate to leave the pixels locked here, and require the caller to pair up the lock/unlock.  IWBN to have some kind of guard class that ensures they're always matched (ignore me if such a thing already exists).
Comment 86 Min Qin 2013-01-28 14:48:22 PST
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.
Comment 87 Min Qin 2013-01-28 14:50:34 PST
Created attachment 185070 [details]
Patch
Comment 88 Min Qin 2013-01-28 15:15:42 PST
Created attachment 185081 [details]
Patch
Comment 89 Min Qin 2013-01-28 15:16:32 PST
Comment on attachment 185049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review

>> Source/WebCore/ChangeLog:8
>> +        This change allows using discardable memory in deferred image decoding path.
> 
> Nit:  in deferred -> in the deferred

Done.

>> Source/WebCore/ChangeLog:11
>> +        Separate caching policy for discardable memory
> 
> This is not a sentence.

This line is covered in the line below, removed.

>> Source/WebCore/ChangeLog:12
>> +        Discardable memory allocation could fail, fall back to heap allocation in that case.
> 
> Nit:  comma splice:  use semicolon instead of comma, or break this into two sentences.

Done.

>> Source/WebCore/ChangeLog:13
>> +        Separate cache size limit for heap entries, no limit on discardable entries
> 
> Same here.  This is also not a sentence.

Done.

>> Source/WebCore/ChangeLog:18
>> +        (WebCore::DiscardablePixelRefAllocator::allocPixelRef):
> 
> You really should have a description of the per-file changes here.

Done.
Comment 90 Xianzhu Wang 2013-01-28 15:34:43 PST
Comment on attachment 185081 [details]
Patch

Will land manually without CQ.
Comment 91 Xianzhu Wang 2013-01-28 15:41:29 PST
Comment on attachment 185081 [details]
Patch

Clearing flags on attachment: 185081

Committed r141020: <http://trac.webkit.org/changeset/141020>
Comment 92 Xianzhu Wang 2013-01-28 15:41:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 93 WebKit Review Bot 2013-01-28 18:16:27 PST
Re-opened since this is blocked by bug 108143
Comment 94 Keishi Hattori 2013-01-28 18:18:41 PST
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
Comment 95 Nat Duca 2013-01-28 18:42:42 PST
I wonder if the WebKit/chromium/DEPS didn't get rolled forward?
Comment 96 Hin-Chung Lam 2013-01-28 18:45:30 PST
I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033

The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API.
Comment 97 noel gordon 2013-01-28 19:00:24 PST
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?
Comment 98 Min Qin 2013-01-28 19:39:37 PST
(In reply to comment #96)
> I submitted an unreviewed build fix: http://trac.webkit.org/changeset/141033
> 
> The use SkMutex is not necessary in this code so remove it would make Mac build happy. The reason for undefined reference is because of component build and SK_API.

We definitely need the SkMutex here. If we don't provide our own mutex, skia will fall back to use a static skmutex arrray. And the skmutex array has 30 mutex in it used in a ring fashion. 
The problem is that 2 DiscardablePixelRef can later on use the same mutex and lead to deadlocks.
Comment 99 Min Qin 2013-01-28 19:42:37 PST
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.
Comment 100 noel gordon 2013-01-28 19:54:10 PST
(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?
Comment 101 Min Qin 2013-01-28 19:58:30 PST
(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.
Comment 103 Min Qin 2013-01-28 20:16:47 PST
(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?
Comment 104 Hin-Chung Lam 2013-01-28 20:22:34 PST
(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.
Comment 105 Hin-Chung Lam 2013-01-28 20:25:40 PST
(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.
Comment 106 Hin-Chung Lam 2013-01-28 20:25:59 PST
(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.
Comment 107 noel gordon 2013-01-28 20:39:32 PST
(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.
Comment 108 Min Qin 2013-01-28 20:42:28 PST
(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.
Comment 109 noel gordon 2013-01-28 21:22:28 PST
(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.
Comment 110 Min Qin 2013-01-28 21:28:45 PST
(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
Comment 111 James Robinson 2013-01-28 21:29:20 PST
I agree that an ASSERT() is silly and not needed here.
Comment 112 noel gordon 2013-01-28 21:31:18 PST
Thank you.  There should also be a way to test the allocAndLockDiscardableMemory failing case, such as with your mock unit test, no?
Comment 113 Hin-Chung Lam 2013-01-28 21:37:31 PST
(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.
Comment 114 Min Qin 2013-01-28 21:55:54 PST
(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.
Comment 115 Min Qin 2013-01-28 22:11:42 PST
(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.