Summary: | [JSC]ArrayBufferView and its ArrayBuffer are appended to object pool in wrong order | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | 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
Yong Li
2012-03-23 14:10:17 PDT
got a fix, will try to create a test (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. (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. (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? (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 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? patch is forthcoming Created attachment 133858 [details]
the patch
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 (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. 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 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 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.
Created attachment 134437 [details]
The fix
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) Created attachment 135646 [details]
Patch for landing
Created attachment 135647 [details]
One more
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 (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... Created attachment 135650 [details]
Typo fixed
Comment on attachment 135650 [details] Typo fixed Clearing flags on attachment: 135650 Committed r113233: <http://trac.webkit.org/changeset/113233> All reviewed patches have been landed. Closing bug. |