Bug 82090 – [JSC]ArrayBufferView and its ArrayBuffer are appended to object pool in wrong order

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Yong Li]

if (!arrayBufferObj || !arrayBufferObj->inherits(&JSArrayBuffer::s_info))
 1241 return false; 

It assumes arrayBufferObj must be a JSArrayBuffer, however arrays like JSUitn8Array are derived from JSArrayBufferView.

[Yong Li]

got a fix, will try to create a test

[Dmitry Lomov]

(In reply to comment #1)
> got a fix, will try to create a test

What is the test that fails for you? 
LayoutTests/fast/canvas/webgl/array-message-passing.html is the key test that tests this functionality and it passes.

[Yong Li]

(In reply to comment #2)
> (In reply to comment #1)
> > got a fix, will try to create a test
> 
> What is the test that fails for you? 
> LayoutTests/fast/canvas/webgl/array-message-passing.html is the key test that tests this functionality and it passes.

Sorry my first conclusion is wrong. It appears like ArrayBufferView just because the ArrayBuffer isn't there, and it hits the other ArrayBufferView.

The reason that the ArrayBuffer isn't there is due to this:

                if (!startObjectInternal(obj)) // handle duplicates
636:                    return true;
                write(ArrayBufferTag);

When the deserializer reads the ArrayBufferView, it doesn't expect missing array buffer.

[Dmitry Lomov]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > got a fix, will try to create a test
> > 
> > What is the test that fails for you? 
> > LayoutTests/fast/canvas/webgl/array-message-passing.html is the key test that tests this functionality and it passes.
> 
> Sorry my first conclusion is wrong. It appears like ArrayBufferView just because the ArrayBuffer isn't there, and it hits the other ArrayBufferView.
> 
> The reason that the ArrayBuffer isn't there is due to this:
> 
>                 if (!startObjectInternal(obj)) // handle duplicates
> 636:                    return true;
>                 write(ArrayBufferTag);
> 
> When the deserializer reads the ArrayBufferView, it doesn't expect missing array buffer.

Could you give an example of JavaScript code that fails?

[Yong Li]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > got a fix, will try to create a test
> > > 
> > > What is the test that fails for you? 
> > > LayoutTests/fast/canvas/webgl/array-message-passing.html is the key test that tests this functionality and it passes.
> > 
> > Sorry my first conclusion is wrong. It appears like ArrayBufferView just because the ArrayBuffer isn't there, and it hits the other ArrayBufferView.
> > 
> > The reason that the ArrayBuffer isn't there is due to this:
> > 
> >                 if (!startObjectInternal(obj)) // handle duplicates
> > 636:                    return true;
> >                 write(ArrayBufferTag);
> > 
> > When the deserializer reads the ArrayBufferView, it doesn't expect missing array buffer.
> 
> Could you give an example of JavaScript code that fails?

I'll look deeper into it. For now let me close it. If there is a real issue, I'll reopen this bug

[Yong Li]

Dmitry, the problem I'm seeing is:

1) when serializing ArrayBufferView, it appends the ArrayBufferView object to m_objectPool first, and then appends the ArrayBuffer object.

2) When deserializing, it reads ArrayBuffer first (to m_gcBuffer), and then reads ArrayBufferView.

So if the objects are referenced later with ObjectReferenceTag, it could mess up.

Trying to get a test case now. Do you see anything wrong above?

[Yong Li]

patch is forthcoming

[Yong Li]

Created attachment 133858 [details]
the patch

[Oliver Hunt]

Did this bug cause a crash or did it just fail to copy correctly?  I'm asking simply because it's important that the deserialiser never crash, even on bogus data

[Yong Li]

(In reply to comment #9)
> Did this bug cause a crash or did it just fail to copy correctly?  I'm asking simply because it's important that the deserialiser never crash, even on bogus data

I believe it fails to copy correctly. My Qt build does crash (on ubuntu 32) though, when trying to report a JS error. I haven't looked deeper into that crash but I think it is another issue. When building with the patch, it doesn't crash any more and can dump the results.

[Yong Li]

to (In reply to comment #10)
> (In reply to comment #9)
> > Did this bug cause a crash or did it just fail to copy correctly?  I'm asking simply because it's important that the deserialiser never crash, even on bogus data
> 
> I believe it fails to copy correctly. My Qt build does crash (on ubuntu 32) though, when trying to report a JS error. I haven't looked deeper into that crash but I think it is another issue. When building with the patch, it doesn't crash any more and can dump the results.

Forgot to mention, it doesn't on QNX

[Dmitry Lomov]

Comment on attachment 133858 [details]
the patch

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

Thanks for fishing out the root cause! It is a bug indeed!
However I do not think that your fix is correct.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1499
> +            m_gcBuffer.removeLast();

I do not understand this. There is no guarantee that m_gcBuffer.last() is in fact a slot for ArrayBuffer. Here is the object serialization of which will fail:
{ new DataView(arrayBuffer), intermediateDataObject, new DataView(arrayBuffer) }
By the second DataView is deserialized, m_gcBuffer.last() will be a slot for intermediateDataObject. 
The real fix would be to synchronize index allocation for serialization and deserialization, the easiest way to do this is on serialization time - first allocate id fot ArrayBuffer and only then allocate id for ArrayBufferView.
I can prep a fix for that if you want (will be today later in the day)

[Yong Li]

Comment on attachment 133858 [details]
the patch

Thanks Dmitry! You are definitely the best one to fix it, at least better than me :). I'll leave it to you then.

[Dmitry Lomov]

Created attachment 134437 [details]
The fix

[Kenneth Russell]

Comment on attachment 134437 [details]
The fix

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

Looks good. Couple of minor typos in the ChangeLogs; feel free to fix upon landing. r=me

> LayoutTests/ChangeLog:5
> +        The implementation of structured cloning algorithm (http://www.w3.org/TR/html5/common-dom-interfaces.html#internal-structured-cloning-algorithm)

Perhaps this ChangeLog message shouldn't duplicate that in WebCore?

> LayoutTests/ChangeLog:15
> +        the ArrayBufferView and then to underlying ArrayBuffer; however on deserialziation the ids were assigned another way round.

typo: deserialization

> LayoutTests/ChangeLog:17
> +        This patch fixes that by assigning the id first to ArrayBuffer and then to ArrayBufferView, and addes corresponding test cases.

typo: adds

> Source/WebCore/ChangeLog:15
> +        the ArrayBufferView and then to underlying ArrayBuffer; however on deserialziation the ids were assigned another way round.

typo (see above)

> Source/WebCore/ChangeLog:17
> +        This patch fixes that by assigning the id first to ArrayBuffer and then to ArrayBufferView, and addes corresponding test cases.

typo (see above)

[Dmitry Lomov]

Created attachment 135646 [details]
Patch for landing

[Dmitry Lomov]

Created attachment 135647 [details]
One more

[Kenneth Russell]

Comment on attachment 135647 [details]
One more

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

r=me again; one minor typo.

> LayoutTests/ChangeLog:5
> +        Adds tests that cover mnore than one view of the same ArrayBuffer being cloned.

typo: mnore

[Dmitry Lomov]

(In reply to comment #18)
> (From update of attachment 135647 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135647&action=review
> 
> r=me again; one minor typo.
> 
> > LayoutTests/ChangeLog:5
> > +        Adds tests that cover mnore than one view of the same ArrayBuffer being cloned.
> 
> typo: mnore

Rats I cant type...

[Dmitry Lomov]

Created attachment 135650 [details]
Typo fixed

[WebKit Review Bot]

Comment on attachment 135650 [details]
Typo fixed

Clearing flags on attachment: 135650

Committed r113233: <http://trac.webkit.org/changeset/113233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.