Bug 63481

Comment on attachment 98781 [details] Patch and tests. Looks cool. Did you mean to nominate this patch for review?

Created attachment 98802 [details] Patch and tests (and ChangeLog update).

Comment on attachment 98802 [details] Patch and tests (and ChangeLog update). Attachment 98802 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8949425

Created attachment 98813 [details] Fix release build (local variable appeared only in an assert.) Before I make (any?) changes to the jsc version, I want to make sure that there's some consensus about what the results of the tests should be.

Looked quickly through the patch, offline with Luke... Here are some quick takeaways: - lets look at what Mozilla does here for test results, especially where spec is not 100% prescriptive. - lets remove 'experimental' implementation of hash table (currently under #ifdef) since it is using a peek into internals of JS objects via reinterpret casts. It is essentially a proposal for new V8 API, so it can be discussed separately from this patch with V8 team. The 'safe' variant is landable, although it may be not the fastest implementation. As long as it does not slow down existing benchmarks (do we have any?) it's fine. - The test which is using "script test' infrastructure but is not a script test can be converted to script test. - it would be nice to get a class comment on the object table to explain what does it solve and how. I'm going to r- the current patch, but it's a very nice one!

Created attachment 99184 [details] Address comments; handle old wire format.

View in context: https://bugs.webkit.org/attachment.cgi?id=99184&action=review Nice patch! First round of comments - I went through the code but haven't looked at tests very closely yet. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:89 > +// address returned by a Handle's operator* is related to the handle, not to the object it holds. I think these comments are written under the influence of another implementation of the same concept :) I suggest talking less about the implementation details and more about how the type should be used. How about this rewording: V8ObjectMap implements a map from v8 objects to arbitrary values. V8 objects cannot be used as keys in ordinary wtf::HashMap - use this class instead. The type argument for GcObject template parameter should be a v8::Object or its inheritor. Suggested usage: V8ObjectMap<v8::Object, int> map; v8::Handle<v8::Object> obj = ...; map.set(obj, 42); > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:96 > +// considers a v8::String to be a v8::Primitive). I think the implementation comments should go inside the class definition braces. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:99 > +// faster native-side maps with JavaScript objects as keys. I do not think it is the sole purpose of this wrapper. We can add a separate comment describing possible alternate implementation. It should go inside the class definition as well. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:102 > +class V8GcHandleMap { I do not like this name: this is not a map from handles, the keys of this maps are v8 objects. Let's call it V8ObjectMap? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:115 > + static const bool safeToCompareToEmptyOrDeleted = false; Unused? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:127 > + bool tryGet(const v8::Handle<GcObject>& handle, T& valueOut) Do not use non-const reference parameters. Use T* for valueOut type. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:130 > + valueOut = m_map.get(*handle); Use HashMap::find to avoid double lookup: HandleToT::iterator result = m_map.find(*handle); if (result != m_map.end()) { *valueOut = result->second(); return true; } return false; > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:168 > ObjectTag = '{', The wire format is becoming much more complex: a comment describing it in more detail than (tag, optional data) will be very useful. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:340 > +#endif nit: move #endif beyond the ASSERT - makes the purpose of this local variable clearer > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:974 > + ++m_nextObjectId; Current implementation does not "deduplicate" strings and any implementation of deduplication will bump up wire format version. Let's get rid of this objectID allocation > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1006 > + || jsObject->IsCallable()) The condition in this if statement represents V8 bindings understanding of what is a "host object" as defined per spec. I think this should be extracted into a separate IsHostObject function in V8Bindings.h. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1021 > + virtual bool consumeTopOfStack(v8::Handle<v8::Value>&) = 0; use pointers instead of references for out arguments. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1025 > + virtual bool prepareEmptyArray(uint32_t length) = 0; nit: maybe call these startArray and startObject, to match finishArray and finishObject? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1342 > + RefPtr<ArrayBuffer> arrayBuffer = ArrayBuffer::create(const_cast<void*>(bufferStart), byteLength); yuck - const casts! Any reason why ArrayBuffer::create doesn't take const void *? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1715 > + bool closeComposite(v8::Handle<v8::Value>& obj) Use pointers instead of references for out arguments > Source/WebCore/dom/ExceptionCode.h:-67 > -#endif Why the above removed?

(In reply to comment #6) > Created an attachment (id=99184) [details] > Address comments; handle old wire format. View in context: https://bugs.webkit.org/attachment.cgi?id=99184&action=review Few more comments, focusing on tests. I'll do another pass on tests later - the coverage looks solid, but I want to think of it a bit more. What is the story for modified layout tests and their expectations on non-V8 bindings? > LayoutTests/ChangeLog:7 > + - Introduces the new HTML5 DOM error codes for TIMEOUT_ERR, INVALID_NODE_TYPE_ERR, DATA_CLONE_ERR Hmm I do not see INVALID_NODE_TYPE_ERR in http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#exceptions. Only introduce what you use in this patch (i.e. just add DATA_CLONE_ERR with its proper code) > LayoutTests/fast/canvas/webgl/script-tests/array-message-passing.js:25 > + testFailed(tname + ": expected " + s.byteLength + " bytes, got " + g.byteLength); Add description to assert message to differentiate between other similar asserts (": expected byteLength " .....) > LayoutTests/fast/canvas/webgl/script-tests/array-message-passing.js:70 > + testFailed(tname + ": expected " + s.BYTES_PER_ELEMENT + ", saw " + g.BYTES_PER_ELEMENT); Add description to the assert: "expected BYTES_PER_ELEMENT" (to differentiate with similar asserts) > LayoutTests/fast/dom/Window/script-tests/postmessage-test.js:118 > + if (message == "evalThunk") { Why this is an if in default: case and not a separate case? > LayoutTests/fast/dom/Window/script-tests/postmessage-test.js:145 > +window.tryPostMessage = function(message, shouldThrow, expected, expectedException) { Rename expectedException into expectedExceptionOrEvalThunk. Then in the beginning of the function add: if (expected == "evalThunk") { var evalThunk = expectedExceptionOrEvalThunk; } else { var expectedException = expectedExceptionOrEvalThunk } Use these variables henceforth.

Created attachment 99884 [details] Rebase and address comments. Thanks for your comments! Bigger deltas since last patch: - Strings are no longer allocated object IDs. - isHostObject added to V8Binding.h/cpp and used in SerializedScriptValue.cpp - Non-v8::Objects that are not special-cased now throw DATA_CLONE_ERR - WebKit vs. Chromium expectations addressed Concerning the changes to DOMException: I'm looking at http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#exception-domexception which includes these missing enumeration values. Leaving the other two out in particular would leave weird holes in ExceptionCode.cpp. The constants in ExceptionCode.h were removed after I consulted with Jian Li (they were getting in the way of constants I needed to define).

(In reply to comment #9) > Concerning the changes to DOMException: I'm looking at http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#exception-domexception which includes these missing enumeration values. Leaving the other two out in particular would leave weird holes in ExceptionCode.cpp. The constants in ExceptionCode.h were removed after I consulted with Jian Li (they were getting in the way of constants I needed to define). It is not a big deal, but here is what I think - this set of exception codes is still an unapproved draft. If you add an exception code that you are not using and which is unrelated to cloning patch, and that exception code later changes, and somebody will be working on those changes, there will be some puzzlement and investigation - why this exception code is added? who uses it? So I am arguing for just adding what you use. At the very least, add a comment saying that exception codes are unused and you are just adding them for completeness.

Comment on attachment 99884 [details] Rebase and address comments. View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review Code changes look good to me, with a minor comment on comment. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:159 > +// externally. I think it should be mentioned that it is ok to persist SerializedScriptValue as a binary blob, but this code should always be used to interpret the blob.

(In reply to comment #11) > (From update of attachment 99884 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review > > Code changes look good to me, with a minor comment on comment. > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:159 > > +// externally. > > I think it should be mentioned that it is ok to persist SerializedScriptValue as a binary blob, but this code should always be used to interpret the blob. (I'll have another good look at the tests soon)

Comment on attachment 99884 [details] Rebase and address comments. View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review Some initial (minor) comments. I need to stare at the code a bit more tonight. > LayoutTests/ChangeLog:11 > + Usually there is only the bug title here (or a very short description). > LayoutTests/ChangeLog:15 > + This is where you would put more details relevant to this part of the chnage. I think the majority of these comments apply to the changes done in WebCore, so I'd recommend factoring your comments appropriately. > Source/WebCore/ChangeLog:8 > + - Amends tests and introduces new ones to check for correctness (the bulk of the patch) This patch is really three or four different things. It would have been nice to structure the patches that way to reduce the size and make the reviewing potentially faster. For example, adding the error codes could be one patch. Adding versioning another patch. Etc. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:124 > +public: public nearly always is the first thing in classes and private, the second. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:164 > +// There is a ref table that maps uint32_ts to v8::Values. uint32_t's? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1400 > + if (shortLength * sizeof(int16_t) != byteLength) Should be sizeof(uint16_t)

(In reply to comment #13) > (From update of attachment 99884 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review > > Source/WebCore/ChangeLog:8 > > + - Amends tests and introduces new ones to check for correctness (the bulk of the patch) > > This patch is really three or four different things. > > It would have been nice to structure the patches that way to reduce the size and make the reviewing potentially faster. > > For example, adding the error codes could be one patch. Adding versioning another patch. Etc. I wasn't trying to get you to split it up at this point. I was just giving warning in advance of any future patches (for other bugs) in WebKit. :)

Comment on attachment 99884 [details] Rebase and address comments. View in context: https://bugs.webkit.org/attachment.cgi?id=99884&action=review A few comments some where giving an overview would be nice. This would give context on why there needs to be things like GenerateFreshObjectTag, so one can read the code and get a better feel for how things are suppose to flow. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:185 > + ArrayTag = '[', // length:uint32_t -> pops the last array from the open stack; I wonder if it is good to have the implementation details here instead of at the serialization code (when it would help explain the code and hopefully get change more readily if the impl changes). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:222 > +// Increment this for each change to the wire format. For each incompatible change? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:949 > + return objectId; Return value never used. Consider removing. What is the meaning of "greyObject"? I'm sure it is something familiar to folks who work on GC/VM but not to me. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1181 > + // ArrayBuffers are always written before dependant ArrayBufferViews. dependent (since we use color instead of colour :) ). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1232 > + --m_position; This seems yucky to embed the knowledge of how many bytes were read during readTag. Perhaps there should be a peekTag instead? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1586 > + virtual bool consumeTopOfStack(v8::Handle<v8::Value>* obj) Use object (avoid abbreviations in WebKit code). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1595 > + virtual bool finishArray(uint32_t length, v8::Handle<v8::Value>* value) Calling this finishArray makes it sound like it is matched with a startArray but it isn't always. When the version is 0, only finishArray will be called. Ditto for finishObject and finishSparseArray. This issue naming makes it harder to understand the code (and more likely for future mistake when folks naturally assume that these calls will be paired). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1656 > + virtual const v8::Handle<v8::Value>& pushObjectReference(const v8::Handle<v8::Value>& obj) s/obj/object/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1659 > + return obj; No one uses the return value. Why have it? (I understand the composability in theory but best to add it if/when needed). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1662 > + virtual bool getObjectReference(uint32_t ref, v8::Handle<v8::Value>* obj) s/ref/reference/ s/obj/object/ This gets an object from a reference, right? Right now the function sounds like it returns a object reference. Also, it doesn't return the object at all but a bool. convertReferenceToObject? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1717 > + uint32_t openNewComposite(const v8::Local<v8::Value>& obj) s/obj/object/ Why openNewComposite as opposed to just openComposite? (The new seems implied. Also, I look for symmetry in the open and close calls and "new" throws this off.) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1719 > + uint32_t newid = m_objectPool.size(); newId > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1722 > + return newid; The return value is never used. Why have it? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1725 > + bool closeComposite(v8::Handle<v8::Value>* obj) s/obj/object/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1729 > + uint32_t oldid = m_objrefStack[m_objrefStack.size() - 1]; oldId (capital I). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1740 > + Vector<uint32_t> m_objrefStack; objectReferenceStack? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1848 > + if (status == Serializer::DataCloneError) { Why not change this to be a "switch (status)" and collapse the Serializer::InputError and Serializer::DataCloneError cases? Actually I'd suggest a quick separate patch to make this a switch (status).

Created attachment 100699 [details] Rebase and address comments. Thanks again for your comments! I've attached a new version of the patch that should address them. > What is the meaning of "greyObject"? I'm sure it is something familiar to folks who work on GC/VM but not to me. I've added an explanatory comment; in defense of the name, commonly in tree or graph traversal algorithms nodes that you haven't yet visited are white; nodes that you are in the process of visiting are grey; nodes that you have finished visiting are black.

Comment on attachment 100699 [details] Rebase and address comments. View in context: https://bugs.webkit.org/attachment.cgi?id=100699&action=review > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:93 > +template<typename GcObject, typename T> GCObject > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:126 > + // v8::Objects, this V8GcHandleMap cannot be used to map v8::Strings to T (because the public V8 API V8GcHandleMap undefined reference. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:754 > return newState; Why isn't m_serializingAccessor reset before this return? (You explained but it would be nice to add a comment because it looks wrong.) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:911 > + return false; return 0? (false doesn't go with StateBase*) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:923 > + greyObject(object); Why can't the greyObject call be done as the 1st thing in this method? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:948 > + // Marks object as having been visited by the serializer and assigns it a unique object ID. Why do you call it a "unique object ID" here but call it am objectPoolOffset on line 976? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:975 > + if ((value->IsObject() || value->IsDate() || value->IsRegExp()) Why are we putting things other than object, date, and regexp into m_objectPool but not writing out object offsets for them? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:984 > + writeNull(); Why did this become a method? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1076 > + if (refTableSize != creator.objectReferenceCount()) It would be nice to add a comment here about what likely went wrong to cause this. A missing call to creator.pushObjectReference in the deserialization (or an extra one) that causes a mismatch between the serialization and deserialization. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1111 > + if (m_version > 0) I think all of these checks for m_version > 0 in this case: if (m_version > 0) creator.pushObjectReference(*value); are simply optimizations for when we read version 0 formats. Since that will be a rare case and these checks clutter the code, why not remove them and simply have creator.pushObjectReference(*value); instead? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1186 > + // ArrayBuffers are always written before dependent ArrayBufferViews. // ArrayBufferView serialization writes the ArrayBuffer before the ArrayBufferView. ? Why is it importnat to know that here? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1264 > + bool readSubTag(ArrayBufferViewSubTag* tag) "readSubTag" sounds too generic for what it does. (It only reads ArrayBufferView sub tags). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1857 > + case Serializer::DataCloneError: Why not just have: case Serializer::InputError: case Serializer::InputError: ...(same code for both) return; > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1867 > + default: case Serializer::Success: > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1869 > + m_data = String(StringImpl::adopt(writer.data())).crossThreadString(); return; case Serializer::JSException; // We should never get here because it was handled above. break; } (By removing the default, you'll get the compiler warnings for unhandled enum values which is nice.) ASSERT_NOT_REACHED();

Comment on attachment 100699 [details] Rebase and address comments. View in context: https://bugs.webkit.org/attachment.cgi?id=100699&action=review ok comments on the tests now. > LayoutTests/fast/dom/Window/script-tests/postmessage-clone-deep-array.js:1 > +document.getElementById("description").innerHTML = "Tests that we abort cloning deep(ish) arrays."; Why does this get a different error than really deep array? > LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js:98 > + doPassFail(v[2] === undefined, "ix 2 undefined"); // undefined ix = index, right? (pls xpnd) > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:1 > +window.jsTestIsAsync = true; Why is this a chromium test? If any port implementing webgl and array message passing should pass this, then it shouldn't be in a chromium directory. > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:2 > +window.cheat = false; When is cheat == true? I think this is debugging code that should be removed (along with the if below). > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:7 > +function classCompare(tname, g, s) { s/tname/testName/ What do g and s stand for? > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:8 > + var classStr = Object.prototype.toString; avoid abbreviations: classString Also ideally you'd stick with stand WebKit 4 space indents. > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:9 > + var gclass = classStr.call(g); gClass (but hopefully with a better substitue for "g"). > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:98 > +function createTypedArrayOverBuffer(tatype, taeltsize, length, substart, sublength) { s/tatype/typedArrayType/ s/taeltsize/typedArray??Size/ > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:140 > + {return [t[0] + "_0b", createTypedArrayOverBuffer(t[1], t[2], 0), typedArrayCompare];})); I wish the "b" of 0b was expanded so I didn't have to try to figure out what it means. > LayoutTests/platform/chromium/fast/dom/Window/script-tests/postmessage-clone-ex.js:1 > +document.getElementById("description").innerHTML = "Tests that we clone object hierarchies"; Why is this chromium only and why does it appear to copy so much from the other similar test?

(In reply to comment #18) > (From update of attachment 100699 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100699&action=review > > ok comments on the tests now. > > > LayoutTests/fast/dom/Window/script-tests/postmessage-clone-deep-array.js:1 > > +document.getElementById("description").innerHTML = "Tests that we abort cloning deep(ish) arrays."; > > Why does this get a different error than really deep array? The description was incorrect. deep-array passes, but there's a weird exception that happens when the harness tries to print the value to say that it passed (because the array is too deep for the default printer). JSC and V8 differ by a period in the error text, hence the different expectations. really-deep-array is intended to trigger the overflow behavior and is correctly described. JSC doesn't currently use DATA_CLONE_ERR (code 25), hence the difference in expectation text for really-deep-array. > > > LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js:98 > > + doPassFail(v[2] === undefined, "ix 2 undefined"); // undefined > > ix = index, right? (pls xpnd) Done. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:1 > > +window.jsTestIsAsync = true; > > Why is this a chromium test? This behavior is not currently in the standard but is there to reflect inadvertent behavior from the previous version of the code that users have come to expect. That being said, I fully expect that this test will apply to the standard once it's completed; at that point we can promote it to a WebKit-global test. > > If any port implementing webgl and array message passing should pass this, then it shouldn't be in a chromium directory. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:2 > > +window.cheat = false; > > When is cheat == true? I think this is debugging code that should be removed (along with the if below). Correct; done. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:7 > > +function classCompare(tname, g, s) { > > s/tname/testName/ > > What do g and s stand for? Done (got/sent). > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:8 > > + var classStr = Object.prototype.toString; > > avoid abbreviations: classString > > Also ideally you'd stick with stand WebKit 4 space indents. Done. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:9 > > + var gclass = classStr.call(g); > > gClass (but hopefully with a better substitue for "g"). Done (gotClass). > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:98 > > +function createTypedArrayOverBuffer(tatype, taeltsize, length, substart, sublength) { > > s/tatype/typedArrayType/ > s/taeltsize/typedArray??Size/ Done (Element). > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:140 > > + {return [t[0] + "_0b", createTypedArrayOverBuffer(t[1], t[2], 0), typedArrayCompare];})); > > I wish the "b" of 0b was expanded so I didn't have to try to figure out what it means. Done (_buffer); others expanded too. > > > LayoutTests/platform/chromium/fast/dom/Window/script-tests/postmessage-clone-ex.js:1 > > +document.getElementById("description").innerHTML = "Tests that we clone object hierarchies"; > > Why is this chromium only and why does it appear to copy so much from the other similar test? My bad. It was a leftover from when I was figuring out how to write platform tests. Nothing referenced it and it has been garbage collected. (In reply to comment #18) > (From update of attachment 100699 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100699&action=review > > ok comments on the tests now. > > > LayoutTests/fast/dom/Window/script-tests/postmessage-clone-deep-array.js:1 > > +document.getElementById("description").innerHTML = "Tests that we abort cloning deep(ish) arrays."; > > Why does this get a different error than really deep array? > > > LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js:98 > > + doPassFail(v[2] === undefined, "ix 2 undefined"); // undefined > > ix = index, right? (pls xpnd) > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:1 > > +window.jsTestIsAsync = true; > > Why is this a chromium test? > > If any port implementing webgl and array message passing should pass this, then it shouldn't be in a chromium directory. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:2 > > +window.cheat = false; > > When is cheat == true? I think this is debugging code that should be removed (along with the if below). > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:7 > > +function classCompare(tname, g, s) { > > s/tname/testName/ > > What do g and s stand for? > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:8 > > + var classStr = Object.prototype.toString; > > avoid abbreviations: classString > > Also ideally you'd stick with stand WebKit 4 space indents. > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:9 > > + var gclass = classStr.call(g); > > gClass (but hopefully with a better substitue for "g"). > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:98 > > +function createTypedArrayOverBuffer(tatype, taeltsize, length, substart, sublength) { > > s/tatype/typedArrayType/ > s/taeltsize/typedArray??Size/ > > > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:140 > > + {return [t[0] + "_0b", createTypedArrayOverBuffer(t[1], t[2], 0), typedArrayCompare];})); > > I wish the "b" of 0b was expanded so I didn't have to try to figure out what it means. > > > LayoutTests/platform/chromium/fast/dom/Window/script-tests/postmessage-clone-ex.js:1 > > +document.getElementById("description").innerHTML = "Tests that we clone object hierarchies"; > > Why is this chromium only and why does it appear to copy so much from the other similar test?

Created attachment 100737 [details] Address comments (re: code and tests both).

Comment on attachment 100737 [details] Address comments (re: code and tests both). (sent with wrong MIME type somehow?)

Created attachment 100738 [details] Address comments (re: code and tests both); attempt to send as text/plain.

(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 100699 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=100699&action=review > > > > ok comments on the tests now. > > > > > LayoutTests/fast/dom/Window/script-tests/postmessage-clone-deep-array.js:1 > > > +document.getElementById("description").innerHTML = "Tests that we abort cloning deep(ish) arrays."; > > > > Why does this get a different error than really deep array? > > The description was incorrect. deep-array passes, but there's a weird exception that happens when the harness tries to print the value to say that it passed (because the array is too deep for the default printer). JSC and V8 differ by a period in the error text, hence the different expectations. really-deep-array is intended to trigger the overflow behavior and is correctly described. (Super low priority but) Want to patch one of them to make the error text the same? (Different results are painful.)

Comment on attachment 100738 [details] Address comments (re: code and tests both); attempt to send as text/plain. View in context: https://bugs.webkit.org/attachment.cgi?id=100738&action=review > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:413 > + void writeObjectOffset(uint32_t offset) objectReference seems like the most commonly used term. s/writeObjectOffset/writeObjectReference/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:438 > + void writeCheckRef(uint32_t numberOfRefs) avoid abbreviations:s/Ref/Reference/ (for both instances). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:440 > + append(CheckRefTag); Perhaps s/CheckRef/ReferenceCount/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:560 > + , m_nextObjectId(0) objectReference seems like the most commonly used term, so s/ObjectId/ObjectReference/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:755 > + // and is checked upon entry in Serializer::doSerialize. I would get rid of the 2nd sentence (because it could easily get out of date). I would consider making the 1st sentence a comment above where the member variable is declared. // Used along with execDepth() to determine the number of // accessors under which the serializer is currently serializing. bool m_isSerializingAccessor; (I changed the name here on purpose so that it clear that it is a bool and what question it answers.) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:764 > + // This m_serializingAccessor reset is necessary for those times when we are serializing I don't think you need a comment for this case. (It is expected -- The odd thing is not reseting during a return. That is a typical buggy pattern so that comment is useful.) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:964 > + uint32_t objectId = m_nextObjectId++; objectReference seems like the most commonly used term, so s/objectId/objectReference/ > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1738 > + uint32_t newId = m_objectPool.size(); objectReference seems like the most commonly used term, so s/newId/newObjectReference/ (I'm not sure that we need "new" in this name. Up to you.) > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1747 > + uint32_t oldId = m_objectReferenceStack[m_objectReferenceStack.size() - 1]; objectReference seems like the most commonly used term, so s/old/oldObjectReference/ (I'm not sure that we need "old" in this name. Up to you.)

Comment on attachment 100738 [details] Address comments (re: code and tests both); attempt to send as text/plain. View in context: https://bugs.webkit.org/attachment.cgi?id=100738&action=review Ok only a few more comments to address and this should be good to go. Thanks! > LayoutTests/platform/chromium/fast/canvas/webgl/array-message-passing.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> "This behavior is not currently in the standard but is there to reflect inadvertent behavior from the previous version of the code that users have come to expect. That being said, I fully expect that this test will apply to the standard once it's completed; at that point we can promote it to a WebKit-global test." I've been thinking about this more and even if we don't follow the standard here. One of our goals in WebKit is to have consistency among ports using WebKit, so I don't think this should be a chromium only test. Frankly this behavior only makes sense and we've seen some sites using it. (For example, this site was one example: http://juliamap.googlelabs.com/#c=0,0$z=0$p=ffffff,ffffff,ffffff,ffffff,ff0000,ffff00,ffff00,ff00,ff$f=mandelbrot). > LayoutTests/platform/chromium/fast/canvas/webgl/script-tests/array-message-passing.js:124 > + ['DataView1_ofs_end', new DataView(createBuffer(1), 1, 0), dataViewCompare], ofs (Please make it more readable). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:385 > + append(DataViewTag); else ASSERT_NOT_REACHED(); In addition, to code that can't have errors detected by the compile, awkward if else structures, this also leads to unnecessary vtables bloat. Do you think the ArrayBufferView::is* should change to an enum value (in a future patch)? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1624 > + array = composite.As<v8::Array>(); What happens if the length given here doesn't agree with the length given when doing "newArray"? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1629 > const int depth = stackDepth() - length; This algorithm in general seems very paranoid (about possible corrupted data), but in this case depth could go negative if given a bad length but it appears ok with that. Should it check?

(In reply to comment #25) > (From update of attachment 100738 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100738&action=review > > Ok only a few more comments to address and this should be good to go. Thanks! > > > LayoutTests/platform/chromium/fast/canvas/webgl/array-message-passing.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > I don't think this should be a chromium only test. When I said this, I don't mean that these tests should be combined into the general test for this. It should be a separate test, just not in platform/chromium alone.

Created attachment 100881 [details] Promote array buffer passing test and address comments. > (array-message-passing) I've moved this into the common tests directory and configured expectations appropriately. > (Super low priority but) Want to patch one of them to make the error text the same? (Different results are painful.) I will look into this; I hope it won't require too many changes to expectations. > Do you think the ArrayBufferView::is* should change to an enum value (in a future patch)? Yes, to permit exactly this kind of typecasing. > What happens if the length given here doesn't agree with the length given when doing "newArray"? > This algorithm in general seems very paranoid (about possible corrupted data), but in this case depth could go negative if given a bad length but it appears ok with that. Should it check? length <= stackDepth() (otherwise we would have returned). Hence: length >= 0 (length is uint32_t), stackDepth() >= 0 (invariant) const int depth = stackDepth() - length; // depth >= 0; depth + length = stackDepth() for (unsigned int i = 0; i < length; ++i) // i = [0..length - 1] array->Set(i, element(depth + i)); // depth + i = [depth .. depth + length - 1] // depth + i = [depth .. stackDepth() - 1] so we'll never try to grab a bad element(). length may <> the length given to newArray if the object is corrupted or if the array was mutated during serialization (and we didn't decide how many elements we were going to write before we started). Our behavior then depends on v8::Object::Set(uint32_t, v8::Handle<Value>), and we're worried about the index >= length case (because otherwise we get the default initialized value). If we assume that the array is still a dense array internally, we'll eventually get down to JSObject::SetElementWithoutInterceptor and JSObject::SetFastElement (v8/src/objects.cc:8263), the effect of which appears to be to rearrange the array's storage to fit the new element (and not to throw an exception or do something even less pleasant). (Thanks to vegorov@ for confirming that this API sticks to JS semantics.) For this patch the above is kind of moot, though, because it does not address the FIXME in newArrayState() from the old code-- so arrays are serialized as sparse arrays and that code doesn't get hit. Another future patch in the making!

Comment on attachment 100881 [details] Promote array buffer passing test and address comments. cq- will land manually since this has a fair chance of causing some port issues (and I need to do a chromium try job as well).

Committed as http://trac.webkit.org/changeset/91300