Issue 2323413002: Support ImageData cloning in the V8-based structured clone path.

jbroman

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-13 13:16:23 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2323413002/20001

4 years, 3 months ago (2016-09-13 13:16:35 UTC) #2

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-13 13:38:26 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/223478)

4 years, 3 months ago (2016-09-13 13:38:27 UTC) #4

jbroman

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-16 00:27:07 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2323413002/40001

4 years, 3 months ago (2016-09-16 00:27:26 UTC) #6

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-16 02:52:59 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/86924)

4 years, 3 months ago (2016-09-16 02:52:59 UTC) #8

jbroman

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-16 13:30:17 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2323413002/60001

4 years, 3 months ago (2016-09-16 13:30:35 UTC) #10

jbroman

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

4 years, 3 months ago (2016-09-16 14:02:53 UTC) #11

haraken

LGTM It's really confusing that ScriptValueSerializer and V8ScriptValueSerializer at the same time. Shall we rename ...

4 years, 3 months ago (2016-09-16 14:17:53 UTC) #13

jbroman

On 2016/09/16 at 14:17:53, haraken wrote: > It's really confusing that ScriptValueSerializer and V8ScriptValueSerializer at ...

4 years, 3 months ago (2016-09-16 14:34:33 UTC) #14

On 2016/09/16 at 14:17:53, haraken wrote:
> It's really confusing that ScriptValueSerializer and V8ScriptValueSerializer
at the same time. Shall we rename the latter to V8ObjectScrializer,
NewScriptValueSerializer or something?

I'm not super happy about it, but ObjectSerializer clashes with the V8 name, the
name SerializedScriptValue, and the fact that it serializes non-object values as
well. "New" seems like it will necessarily require a big renaming after the
fact. Would a class comment explaining the difference help?

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:96:
if (pixelArray->length() < pixelLength)
On 2016/09/16 at 14:17:53, haraken wrote:
> When can this happen?

It should never be produced by V8ScriptValueSerializer, but it could happen if
data is corrupted on disk, or a malicious renderer is trying to cause invalid
memory access by sending a corrupt message to another renderer.

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:114:
wrappable = readDOMObject(tag);
On 2016/09/16 at 14:17:53, haraken wrote:
> Maybe can we move readTag() into readDOMObject()?

readDOMObject will be a virtual method which is overridden for modules (which
will call its superclass method to deal with core objects). However, the tag
should only be read once, so both implementations cannot read it.

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:133:
exceptionState.throwDOMException(DataCloneError, "An object could not be
cloned.");
On 2016/09/16 at 14:17:53, haraken wrote:
> If you want to pass in exceptionState to writeDOMObject(), it would be better
to set DataCloneError in writeDOMObject().

I did it this way with the goal of making it simpler when there's an override of
writeDOMObject. I expect this to look roughly like:

bool V8ScriptValueSerializerForModules::writeDOMObject(ScriptWrappable*
wrappable, ExceptionState& exceptionState)
{
    if (V8ScriptValueSerializer::writeDOMObject(wrappable, exceptionState))
        return true;
    if (exceptionState.hadException())
        return false;
    // check for modules types
    return false;
}

If V8ScriptValueSerializer::writeDOMObject throws an exception for objects which
aren't valid _core_ types, then V8ScriptValueSerializerForModules won't have an
opportunity to return a valid object (unless it clears the exception, but
throwing and then clearing an exception seems weird).

There needs to be a way for V8ScriptValueSerializer to indicate to the derived
class that it was not able to serialize the object -- but not because it saw any
particular error, just because it didn't recognize the kind of object.

haraken

On 2016/09/16 14:34:33, jbroman wrote: > On 2016/09/16 at 14:17:53, haraken wrote: > > It's ...

4 years, 3 months ago (2016-09-16 14:39:45 UTC) #15

On 2016/09/16 14:34:33, jbroman wrote:
> On 2016/09/16 at 14:17:53, haraken wrote:
> > It's really confusing that ScriptValueSerializer and V8ScriptValueSerializer
> at the same time. Shall we rename the latter to V8ObjectScrializer,
> NewScriptValueSerializer or something?
> 
> I'm not super happy about it, but ObjectSerializer clashes with the V8 name,
the
> name SerializedScriptValue, and the fact that it serializes non-object values
as
> well. "New" seems like it will necessarily require a big renaming after the
> fact. Would a class comment explaining the difference help?

Then LegacyScriptValueSerializer and ScriptValueSerializer?

Either way we can fix it in a follow up.

> 
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
> (right):
> 
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:96:
> if (pixelArray->length() < pixelLength)
> On 2016/09/16 at 14:17:53, haraken wrote:
> > When can this happen?
> 
> It should never be produced by V8ScriptValueSerializer, but it could happen if
> data is corrupted on disk, or a malicious renderer is trying to cause invalid
> memory access by sending a corrupt message to another renderer.
> 
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:114:
> wrappable = readDOMObject(tag);
> On 2016/09/16 at 14:17:53, haraken wrote:
> > Maybe can we move readTag() into readDOMObject()?
> 
> readDOMObject will be a virtual method which is overridden for modules (which
> will call its superclass method to deal with core objects). However, the tag
> should only be read once, so both implementations cannot read it.
> 
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
> (right):
> 
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:133:
> exceptionState.throwDOMException(DataCloneError, "An object could not be
> cloned.");
> On 2016/09/16 at 14:17:53, haraken wrote:
> > If you want to pass in exceptionState to writeDOMObject(), it would be
better
> to set DataCloneError in writeDOMObject().
> 
> I did it this way with the goal of making it simpler when there's an override
of
> writeDOMObject. I expect this to look roughly like:
> 
> bool V8ScriptValueSerializerForModules::writeDOMObject(ScriptWrappable*
> wrappable, ExceptionState& exceptionState)
> {
>     if (V8ScriptValueSerializer::writeDOMObject(wrappable, exceptionState))
>         return true;
>     if (exceptionState.hadException())
>         return false;
>     // check for modules types
>     return false;
> }
> 
> If V8ScriptValueSerializer::writeDOMObject throws an exception for objects
which
> aren't valid _core_ types, then V8ScriptValueSerializerForModules won't have
an
> opportunity to return a valid object (unless it clears the exception, but
> throwing and then clearing an exception seems weird).
> 
> There needs to be a way for V8ScriptValueSerializer to indicate to the derived
> class that it was not able to serialize the object -- but not because it saw
any
> particular error, just because it didn't recognize the kind of object.

Sounds reasonable to me.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2323413002/60001

4 years, 3 months ago (2016-09-16 14:42:45 UTC) #18

commit-bot: I haz the power

Description was changed from ========== Support ImageData cloning in the V8-based structured clone path. This ...

4 years, 3 months ago (2016-09-16 15:35:07 UTC) #20

commit-bot: I haz the power

Patchset 4 (id:??) landed as https://crrev.com/e29d78e9bf1079f66fd4c2fc9c0df8701c9d196b Cr-Commit-Position: refs/heads/master@{#419172}

4 years, 3 months ago (2016-09-16 15:35:08 UTC) #21

esprehn

esprehn@chromium.org changed reviewers: + esprehn@chromium.org

4 years, 3 months ago (2016-09-16 18:41:26 UTC) #22

esprehn

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp#newcode91 third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91: if (wrapperTypeInfo == &V8ImageData::wrapperTypeInfo) { fwiw I wonder if ...

4 years, 3 months ago (2016-09-16 18:41:28 UTC) #23

jbroman

https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp#newcode91 third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91: if (wrapperTypeInfo == &V8ImageData::wrapperTypeInfo) { On 2016/09/16 at 18:41:27, ...

4 years, 3 months ago (2016-09-16 19:40:02 UTC) #24

haraken

On 2016/09/16 19:40:02, jbroman wrote: > https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp > File > third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp > (right): > > ...

4 years, 3 months ago (2016-09-17 00:06:33 UTC) #25

haraken

4 years, 3 months ago (2016-09-17 00:07:10 UTC) #26

Message was sent while issue was closed.

On 2016/09/17 00:06:33, haraken wrote:
> On 2016/09/16 19:40:02, jbroman wrote:
> >
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2323413002/diff/60001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91:
> > if (wrapperTypeInfo == &V8ImageData::wrapperTypeInfo) {
> > On 2016/09/16 at 18:41:27, esprehn wrote:
> > > fwiw I wonder if we should use a HashMap of <WrapperTypeInfo*,
> WTF::Function>
> > instead and modules should just register() all the types with the map
instead
> of
> > this virtual override dance. It would mean that modules no longer needs to
> > subclass this thing, and individual modules can just register their
> serializers
> > and the function would take a serializer instance.
> > 
> > I suggested doing that to ScriptValueSerializer a while back, and got push
> back.
> > And to be honest, I now do think it's simpler this way, especially because
> you'd
> > need a similar map (from SerializationTag) on the other side, so you'd have
> each
> > one implementing two WTF::Functions. The natural response to that is to make
> it
> > a class that gets registered with a WrapperTypeInfo* and a SerializationTag.
> > It's not even a perfectly clean abstraction, because some, like Blob and
> > MessagePort, are special snowflakes that need to do particular things
> > (WebBlobInfoArray needs to be exported so that the storage layer can manage
> the
> > blob/indexeddb connection; message port entanglement is currently done
> > specially).
> 
> (I don't have any strong opinion on this. The clearer, the better :-)

s/clearer/cleaner/

Issue 2323413002: Support ImageData cloning in the V8-based structured clone path. (Closed)

Description

Patch Set 1 #

Patch Set 2 : unit tests #

Patch Set 3 : add the isolate parameter #

Patch Set 4 : does win_clang like the other order better? #

Messages