Make ImageBitmap Transferable

Make ImageBitmap Transferable

Including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main.

BUG=532142
Committed: https://crrev.com/5d8ed50933226127fdc64744d86ab3d9b7d57276
Cr-Commit-Position: refs/heads/master@{#356575}

[xidachen, 2015-10-22 13:56:24]

Description was changed from

==========
Make ImageBitmap Transferable

Including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main.

BUG=532142
==========

to

==========
Make ImageBitmap Transferable

Including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main.

BUG=532142
==========

[xidachen, 2015-10-22 13:56:24]

xidachen@chromium.org changed reviewers:
+ chrishtr@chromium.org, jochen@chromium.org, junov@chromium.org

[xidachen, 2015-10-22 13:58:46]

Hi,
This CL is for making ImageBitmap transferable.
Please review.

Thank you.

[jochen (gone - plz use gerrit), 2015-10-23 12:17:42]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2015-10-23 12:17:42]

+haraken for bindings

[Justin Novosad, 2015-10-23 13:30:04]

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-transferable.html
(right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-transferable.html:10:
description("Tests that the ImageBitmap is transferable");
...and that the pixel data survives the trip.

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/resources/canvas-ImageBitmap-transferable.js
(right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/resources/canvas-ImageBitmap-transferable.js:4:
// from the main thread
Explain why. I imagine it is to get test coverage for calling
createImageBitmap(ImageBitmap) in a worker? This aspect should be part of the
test description.

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:580:
ScriptValueDeserializer(SerializedScriptValueReader& reader, MessagePortArray*
messagePorts, ArrayBufferContentsArray* arrayBufferContents, ImageBitmapArray*
imageBitmaps)
No need to fix this right away, but please file a bug for creating a
"Transferable" C++ base class. The current approach won't scale so nicely as
more things become transferable in the future.  FYI: There already is a general
bug for using IDL union types (crbug.com/240176).

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:40: 
No need for extra blank line.

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:60: typedef
Vector<RefPtr<ImageBitmap>, 1> ImageBitmapArray;
This will fail on Oilpan builds because ImageBitmap is a
RefCountedWillBeGarbageCollectedFinalized.  You should probably declare this
something like: WillBeHeapVector<RefPtrWillBeMember<ImageBitmap> >

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:193:
m_bitmapRect.setLocation(IntPoint(0, 0));
Setting the rects and offset to 0 is overkill.

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:202: return adoptRef(new
ImageBitmap(this, m_cropRect));
Not oilpan compatible.

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:46: PassRefPtr<ImageBitmap>
transfer();
PassRefPtrWillBeRawPtr

https://codereview.chromium.org/1409243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:83: bool m_isNeutered;
Is this field necessary? I think isNeuteres can be implementes as "return
!m_bitmap && !m_imageElement"

[xidachen, 2015-10-23 14:41:05]

Hi Justin,

Thank you for the awesome comments. I have fixed the code and it should be
OilPan compatible now.

[Justin Novosad, 2015-10-23 15:01:22]

https://codereview.chromium.org/1409243005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:194: return adoptRef(new
ImageBitmap(this, m_cropRect));
I don't think this is oilpan safe. The adoptRef should be gone in oilpan mode. I
think what you want here is adoptRefWillBeNoop.

[xidachen, 2015-10-23 18:02:22]

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:116:
OwnPtr<ImageBitmapArray> contents = adoptPtr(new
ImageBitmapArray(imageBitmaps.size()));
This may not be an easy fix, should I use OwnPtrWillBePersistent?
haraken@, could you give me some suggestions about this one? I am not sure which
way would be thread safe.

[Justin Novosad, 2015-10-23 19:39:57]

Oilpan is a bit tricky and hard to figure out just by reading code and trial and
error. You should read up on it here: https://www.chromium.org/blink/blink-gc

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:116:
OwnPtr<ImageBitmapArray> contents = adoptPtr(new
ImageBitmapArray(imageBitmaps.size()));
On 2015/10/23 18:02:22, xidachen wrote:
> This may not be an easy fix, should I use OwnPtrWillBePersistent?
> haraken@, could you give me some suggestions about this one? I am not sure
which
> way would be thread safe.

For references that are on the stack, like this one, you should use
OwnPtrWillBeRawPtr. The return type of this function should then be
PassOwnPtrWillBeRawPtr<ImageBitmapArray>

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:130:
OwnPtr<ImageBitmapArray> m_imageBitmapsArray;
Because SerializedScriptValue is ref counted (As opposed to garbage collected)
the guy should be OwnPtrWillBePersistent<ImageBitmapArray>.

[xidachen, 2015-10-24 01:22:33]

Thank you Justin. 

After making changes, the oilpan trybots goes green, and the layout test passes.

[Justin Novosad, 2015-10-24 03:04:48]

https://codereview.chromium.org/1409243005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:184: void
ImageBitmap::neuter()
I don't see why neuter and transfer need to be separate functions. Semantically
transfer means make a shallow copy and neuter the original. Right now transfer()
only does the copy part, which is misleading. IMHO we should just have one
method named 'transfer' that does all the work. If there is a legitimate reason
to have both methods, then they should be named 'neuter' and 'shallowCopy'.

[xidachen, 2015-10-24 20:36:25]

https://codereview.chromium.org/1409243005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:184: void
ImageBitmap::neuter()
That makes perfect sense. They are now merged into one function "transfer".

[haraken, 2015-10-26 06:52:14]

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1409243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:116:
OwnPtr<ImageBitmapArray> contents = adoptPtr(new
ImageBitmapArray(imageBitmaps.size()));
On 2015/10/23 19:39:57, Justin Novosad wrote:
> On 2015/10/23 18:02:22, xidachen wrote:
> > This may not be an easy fix, should I use OwnPtrWillBePersistent?
> > haraken@, could you give me some suggestions about this one? I am not sure
> which
> > way would be thread safe.
> 
> For references that are on the stack, like this one, you should use
> OwnPtrWillBeRawPtr. The return type of this function should then be
> PassOwnPtrWillBeRawPtr<ImageBitmapArray>

Sorry for being late. Yes, correct. This should be:

  OwnPtrWillBeRawPtr<ImageBitmapArray> contents = adoptPtrWillBeNoop(new ...);

The GC plugin will detect these errors at compile time. Can you just try to
compile your CL with GYP_DEFINES='enable_oilpan=1 blink_gc_plugin=1'?

[xidachen, 2015-10-26 13:44:10]

Thank you for your suggestions. I did try building my CL with the parameter
GYP_DEFINES='enable_oilpan=1 blink_gc_plugin=1', and everything works fine. With
all the OilPan trybots results, I am quite sure that the code should be OilPan
compatible now.

[haraken, 2015-10-26 14:57:40]

LGTM from the perspective of bindings/ and Oilpan.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2116: bool
ScriptValueDeserializer::tryGetTransferredImageBitmap(uint32_t index,
v8::Local<v8::Value>* object)

Why is this implementation different from tryGetTransferredArrayBuffer? I guess
these two methods should have the same logic.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:117:
HashSet<ImageBitmap*> visited;

WillBeHeapHashSet<RawPtrWillBeMember<ImageBitmap>>

(This error will be detected by our new verification system soon :-)

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:121:
ArrayBufferContentsArray* arrayBufferContentsArray() { return
m_arrayBufferContentsArray.get(); }

Add const.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:122:
ImageBitmapArray* imageBitmapArray() { return m_imageBitmapsArray.get(); }

Ditto.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:56:
return create(isolate, value, 0, 0, 0, exceptionState);

We prefer using nullptr.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:61:
ASSERT(isolate->InContext());

Ditto.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:112:
serializedValue->transferImageBitmaps(isolate, *imageBitmaps, exceptionState);

I'd refactor the code as follows:

if (imageBitmaps && imageBitmaps->size())
  ...;
else if (arrayBuffers && arrayBuffers->size())
  ...;
else
  return;

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_types.py:513:
'SerializedScriptValue':
'SerializedScriptValueFactory::instance().create({isolate}, {v8_value}, 0, 0, 0,
exceptionState)',

nullptr

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:186:
WTF::RawPtr<ImageBitmap> bitmap = adoptRefWillBeNoop(new ImageBitmap(this,
m_cropRect));

RefPtrWillBeRawPtr

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:191: return bitmap;

bitmap.release()

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSerializedScriptValue.cpp (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSerializedScriptValue.cpp:49:
WebSerializedScriptValue serializedValue =
SerializedScriptValueFactory::instance().create(v8::Isolate::GetCurrent(),
value, 0, 0, 0, exceptionState);

nullptr

[sof, 2015-10-26 16:31:35]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-10-26 16:31:35]

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:635:
ImageBitmapArray* m_imageBitmaps;
This works out overall, but it is not quite compatible with Oilpan stack
allocated objects to use pointers to collection wrapper objects.

You're not adding anything that wasn't here before (cf. line above), so could
you add a TODO:

 TODO(Oilpan): turn these optional heap vector objects into proper fields.

[haraken, 2015-10-26 16:35:39]

haraken@chromium.org changed reviewers:
+ yutak@chromium.org

[haraken, 2015-10-26 16:35:39]

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:635:
ImageBitmapArray* m_imageBitmaps;
On 2015/10/26 16:31:35, sof wrote:
> This works out overall, but it is not quite compatible with Oilpan stack
> allocated objects to use pointers to collection wrapper objects.
> 
> You're not adding anything that wasn't here before (cf. line above), so could
> you add a TODO:
> 
>  TODO(Oilpan): turn these optional heap vector objects into proper fields. 

yutak@: It would be better if the GC plugin can detect this error.

(BTW, as discussed offline with yutak@ a couple of weeks ago, I'm leaning toward
to replacing all Member<>s in stack-allocated objects to raw pointers.)

[sof, 2015-10-26 16:39:22]

On 2015/10/26 16:35:39, haraken wrote:
>
https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
(right):
> 
>
https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:635:
> ImageBitmapArray* m_imageBitmaps;
> On 2015/10/26 16:31:35, sof wrote:
> > This works out overall, but it is not quite compatible with Oilpan stack
> > allocated objects to use pointers to collection wrapper objects.
> > 
> > You're not adding anything that wasn't here before (cf. line above), so
could
> > you add a TODO:
> > 
> >  TODO(Oilpan): turn these optional heap vector objects into proper fields. 
> 
> yutak@: It would be better if the GC plugin can detect this error.
> 
> (BTW, as discussed offline with yutak@ a couple of weeks ago, I'm leaning
toward
> to replacing all Member<>s in stack-allocated objects to raw pointers.)

Not sure I see a direct connnection; HeapVector<> isn't on the heap.

[haraken, 2015-10-26 16:41:05]

On 2015/10/26 16:39:22, sof wrote:
> On 2015/10/26 16:35:39, haraken wrote:
> >
>
https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:635:
> > ImageBitmapArray* m_imageBitmaps;
> > On 2015/10/26 16:31:35, sof wrote:
> > > This works out overall, but it is not quite compatible with Oilpan stack
> > > allocated objects to use pointers to collection wrapper objects.
> > > 
> > > You're not adding anything that wasn't here before (cf. line above), so
> could
> > > you add a TODO:
> > > 
> > >  TODO(Oilpan): turn these optional heap vector objects into proper fields.

> > 
> > yutak@: It would be better if the GC plugin can detect this error.
> > 
> > (BTW, as discussed offline with yutak@ a couple of weeks ago, I'm leaning
> toward
> > to replacing all Member<>s in stack-allocated objects to raw pointers.)
> 
> Not sure I see a direct connnection; HeapVector<> isn't on the heap.

ah, I was confused. Ignore my comment #20.

[Justin Novosad, 2015-10-26 17:12:23]

lgtm for code under Source/core (modulo haraken's comments) and LayoutTests.

[xidachen, 2015-10-26 19:04:07]

Hi everyone, I did a sync to the most recent code base today and my CL crashed
(Without changing my code). I need to investigate into this. Will response as
soon as it is fixed.

[xidachen, 2015-10-27 12:23:24]

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2116: bool
ScriptValueDeserializer::tryGetTransferredImageBitmap(uint32_t index,
v8::Local<v8::Value>* object)
This is a bit tricky, following is my opinion and I hope it is expressed
clearly.
The transferable ArrayBuffer works like this:
It has a V8ArrayBuffer type which is a wrapper for DOMArrayBuffer type, which is
constructed from WTF::ArrayBuffer. When transferring the ArrayBuffer, it
actually transfer the contents of the WTF::ArrayBuffer, which is
ArrayBufferContents type.

When it comes to ImageBitmap, it is different from ArrayBuffer because
ImageBitmap has .idl file. When implementing the serializing and de-serializing
for ImageBitmap, I followed the example of MessagePort which also has .idl file.
I hope this makes sense.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:635:
ImageBitmapArray* m_imageBitmaps;
On 2015/10/26 16:31:35, sof wrote:
> This works out overall, but it is not quite compatible with Oilpan stack
> allocated objects to use pointers to collection wrapper objects.
> 
> You're not adding anything that wasn't here before (cf. line above), so could
> you add a TODO:
> 
>  TODO(Oilpan): turn these optional heap vector objects into proper fields. 

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:117:
HashSet<ImageBitmap*> visited;
On 2015/10/26 14:57:39, haraken wrote:
> 
> WillBeHeapHashSet<RawPtrWillBeMember<ImageBitmap>>
> 
> (This error will be detected by our new verification system soon :-)

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:121:
ArrayBufferContentsArray* arrayBufferContentsArray() { return
m_arrayBufferContentsArray.get(); }
This one is tricky. I tried to add const here, and it causes chain reactions and
I had to change a lot of function signatures by adding const. Eventually it
points to the constructor of "ScriptValueDeserializer", where there will be
error in this line:
m_imageBitmaps(imageBitmaps);
Here, imageBitmaps is the const parameter and it leads to an error.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:56:
return create(isolate, value, 0, 0, 0, exceptionState);
On 2015/10/26 14:57:40, haraken wrote:
> 
> We prefer using nullptr.

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:61:
ASSERT(isolate->InContext());
On 2015/10/26 14:57:40, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp:112:
serializedValue->transferImageBitmaps(isolate, *imageBitmaps, exceptionState);
This is an excellent comment, it makes the code much more readable. 
While I was doing this, I realized that it is possible that both of 
imageBitmaps and arrayBuffers are non-empty, and in this case I think what we
should do is:
if (imageBitmaps && imageBitmaps->size())
  ...;
if (arrayBuffers && arrayBuffers->size())
  ...;

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/v8_types.py:513:
'SerializedScriptValue':
'SerializedScriptValueFactory::instance().create({isolate}, {v8_value}, 0, 0, 0,
exceptionState)',
On 2015/10/26 14:57:40, haraken wrote:
> 
> nullptr

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:186:
WTF::RawPtr<ImageBitmap> bitmap = adoptRefWillBeNoop(new ImageBitmap(this,
m_cropRect));
On 2015/10/26 14:57:40, haraken wrote:
> 
> RefPtrWillBeRawPtr

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:191: return bitmap;
On 2015/10/26 14:57:40, haraken wrote:
> 
> bitmap.release()

Done.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:191: return bitmap;
junov@: I had to add back the code that sets the m_bitmapRect to be 0. The
reason is this: there is a V8ImageBitmap reference pointer that points to this
ImageBitmap object, so even though the m_bitmap is cleared, the V8ImageBitmap
pointer can still see the size of this ImageBitmap object. Actually by looking
back at this CL, I found out that the "expected.txt" is wrong starting from
Patch Set #4, which is because I didn't explicitly set the m_bitmapRect to be 0.
Now that I put it back, the "expected.txt" is correct.

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSerializedScriptValue.cpp (right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSerializedScriptValue.cpp:49:
WebSerializedScriptValue serializedValue =
SerializedScriptValueFactory::instance().create(v8::Isolate::GetCurrent(),
value, 0, 0, 0, exceptionState);
On 2015/10/26 14:57:40, haraken wrote:
> 
> nullptr

Done.

[haraken, 2015-10-27 13:10:33]

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1409243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2116: bool
ScriptValueDeserializer::tryGetTransferredImageBitmap(uint32_t index,
v8::Local<v8::Value>* object)
On 2015/10/27 12:23:24, xidachen wrote:
> This is a bit tricky, following is my opinion and I hope it is expressed
> clearly.
> The transferable ArrayBuffer works like this:
> It has a V8ArrayBuffer type which is a wrapper for DOMArrayBuffer type, which
is
> constructed from WTF::ArrayBuffer. When transferring the ArrayBuffer, it
> actually transfer the contents of the WTF::ArrayBuffer, which is
> ArrayBufferContents type.
> 
> When it comes to ImageBitmap, it is different from ArrayBuffer because
> ImageBitmap has .idl file. When implementing the serializing and
de-serializing
> for ImageBitmap, I followed the example of MessagePort which also has .idl
file.
> I hope this makes sense.

Thanks, makes sense.

[xidachen, 2015-10-28 15:20:13]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2015-10-28 15:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1409243005/#ps160001
(title: "Fix Oilpan and other issues")

[commit-bot: I haz the power, 2015-10-28 15:20:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409243005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409243005/160001

[commit-bot: I haz the power, 2015-10-28 15:28:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-28 15:28:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xidachen, 2015-10-28 15:49:04]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-10-28 15:49:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409243005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409243005/160001

[commit-bot: I haz the power, 2015-10-28 17:19:15]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-28 17:19:57]

Patchset 9 (id:??) landed as
https://crrev.com/5d8ed50933226127fdc64744d86ab3d9b7d57276
Cr-Commit-Position: refs/heads/master@{#356575}

[sof, 2015-10-28 21:44:13]

See
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
for some relevant crashes.

https://codereview.chromium.org/1409243005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1409243005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:130:
OwnPtrWillBePersistent<ImageBitmapArray> m_imageBitmapsArray;
How is intended to work? The transfer happens on one thread, but the
receiver/consumer of this SerializedScriptValue is on another thread, in the
general case. Which means it will destruct the value..and the Persistent<>.

Persistent<> have a same-thread restriction on use (thread that creates ==
thread that destructs); CrossThreadPersistent<> loosens the restriction on who
gets to destruct the persistent reference. But.. is it safe to assume that the
thread that created the ImageBitmapArray remains alive until the
SerializedScriptValue has been fully consumed & finalized? If not, then you risk
that thread's heap from disappearing & any ImageBitmapArray accesses to touch
freed memory. Which isn't so good.

[sof, 2015-10-29 12:27:42]

+Cc: oilpan-reviews@

[xidachen, 2015-10-29 14:20:13]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1413723004/ by xidachen@chromium.org.

The reason for reverting is: There are tests that are crashed:
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil....

[Justin Novosad, 2015-10-29 14:49:52]

On 2015/10/29 14:20:13, xidachen wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1413723004/ by mailto:xidachen@chromium.org.
> 
> The reason for reverting is: There are tests that are crashed:
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil....

I think this can be fixed by not transferring ImageBitmap proper, but rather by
tranfering the SkImage objects that are inside the ImageBitmap. SkImages are not
GarbageCollected. They use atomic skia reference counting. Then new ImageBitmaps
can be safely constructed around the transferred SkImages, directly in the
destination thread.

[sof, 2015-10-29 15:57:15]

On 2015/10/29 14:49:52, Justin Novosad wrote:
> On 2015/10/29 14:20:13, xidachen wrote:
> > A revert of this CL (patchset #9 id:160001) has been created in
> > https://codereview.chromium.org/1413723004/ by mailto:xidachen@chromium.org.
> > 
> > The reason for reverting is: There are tests that are crashed:
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil....
> 
> I think this can be fixed by not transferring ImageBitmap proper, but rather
by
> tranfering the SkImage objects that are inside the ImageBitmap. SkImages are
not
> GarbageCollected. They use atomic skia reference counting. Then new
ImageBitmaps
> can be safely constructed around the transferred SkImages, directly in the
> destination thread.

I do hope that's feasible (but noticed m_imageElement on ImageBitmap, which
seemed to survive the transfer.)

If not, we could consider adding a cross-thread heap for reliably passing
objects between threads -- that heap surviving until the main thread heap shuts
down.

(Thanks for the revert, xidachen@. Sorry for not thinking through the
ramifications of transferring Oilpan objects earlier.)

[xidachen, 2015-10-29 16:08:32]

> I do hope that's feasible (but noticed m_imageElement on ImageBitmap, which
> seemed to survive the transfer.)
> 

This is a good point. I am trying to remove the m_imageElement, that would
simplify things as well.