Bug 82090

Summary: [JSC]ArrayBufferView and its ArrayBuffer are appended to object pool in wrong order
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: WebCore JavaScriptAssignee: Dmitry Lomov <dslomov>
Status: RESOLVED FIXED    
Severity: Normal CC: dslomov, kbr, levin, oliver, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch
none
The fix
kbr: review+, kbr: commit-queue-
Patch for landing
none
One more
kbr: review+, dslomov: commit-queue-
Typo fixed none

Description Yong Li 2012-03-23 14:10:17 PDT
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.
Comment 1 Yong Li 2012-03-23 14:10:44 PDT
got a fix, will try to create a test
Comment 2 Dmitry Lomov 2012-03-23 14:28:44 PDT
(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.
Comment 3 Yong Li 2012-03-23 14:47:08 PDT
(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.
Comment 4 Dmitry Lomov 2012-03-23 16:07:26 PDT
(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?
Comment 5 Yong Li 2012-03-26 07:10:46 PDT
(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
Comment 6 Yong Li 2012-03-26 10:50:38 PDT
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?
Comment 7 Yong Li 2012-03-26 11:47:46 PDT
patch is forthcoming
Comment 8 Yong Li 2012-03-26 12:00:33 PDT
Created attachment 133858 [details]
the patch
Comment 9 Oliver Hunt 2012-03-26 12:22:32 PDT
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
Comment 10 Yong Li 2012-03-26 12:27:46 PDT
(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.
Comment 11 Yong Li 2012-03-26 12:39:33 PDT
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
Comment 12 Dmitry Lomov 2012-03-26 12:44:29 PDT
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)
Comment 13 Yong Li 2012-03-26 13:22:21 PDT
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.
Comment 14 Dmitry Lomov 2012-03-28 16:23:01 PDT
Created attachment 134437 [details]
The fix
Comment 15 Kenneth Russell 2012-04-03 12:29:55 PDT
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)
Comment 16 Dmitry Lomov 2012-04-04 12:11:54 PDT
Created attachment 135646 [details]
Patch for landing
Comment 17 Dmitry Lomov 2012-04-04 12:13:50 PDT
Created attachment 135647 [details]
One more
Comment 18 Kenneth Russell 2012-04-04 12:15:14 PDT
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
Comment 19 Dmitry Lomov 2012-04-04 12:17:58 PDT
(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...
Comment 20 Dmitry Lomov 2012-04-04 12:19:55 PDT
Created attachment 135650 [details]
Typo fixed
Comment 21 WebKit Review Bot 2012-04-04 13:43:49 PDT
Comment on attachment 135650 [details]
Typo fixed

Clearing flags on attachment: 135650

Committed r113233: <http://trac.webkit.org/changeset/113233>
Comment 22 WebKit Review Bot 2012-04-04 13:43:55 PDT
All reviewed patches have been landed.  Closing bug.