We should use v8Undefined() everywhere in V8 bindings. A couple of v8::Undefined()s cannot be simply replaced due to circular dependency (i.e. some files cannot include V8Binding.h). I'll resolve the issue in a follow-up patch.
Created attachment 156583 [details] Patch
Comment on attachment 156583 [details] Patch Attachment 156583 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13433796 New failing tests: animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html http/tests/appcache/deferred-events-delete-while-raising-timer.html http/tests/appcache/crash-when-navigating-away-then-back.html accessibility/media-controls.html accessibility/loading-iframe-sends-notification.html animations/animation-border-overflow.html accessibility/contenteditable-table-check-causes-crash.html animations/animation-direction-alternate-reverse.html animations/3d/state-at-end-event-transform.html accessibility/label-element-press.html canvas/philip/tests/2d.canvas.readonly.html animations/3d/change-transform-in-end-event.html accessibility/loading-iframe-updates-axtree.html accessibility/menu-list-sends-change-notification.html http/tests/appcache/access-via-redirect.php animations/3d/transform-perspective.html animations/animation-direction-reverse-hardware-opacity.html http/tests/appcache/deferred-events.html animations/animation-controller-drt-api.html animations/3d/transform-origin-vs-functions.html http/tests/appcache/destroyed-frame.html animations/animation-direction-reverse-fill-mode-hardware.html animations/animation-direction-reverse-fill-mode.html accessibility/canvas.html http/tests/appcache/detached-iframe.html
Created attachment 156588 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 156591 [details] Patch
Looks like that SetHiddenValue() crashes if we pass Handle<Value>() to the second argument: SetHiddenValue(..., v8::Undefined()); // Works fine. SetHiddenValue(..., Handle<Value>()); // Crashes. I filed a V8 bug: http://code.google.com/p/v8/issues/detail?id=2274 For now, the patch leaves v8::Undefined() as is for SetHiddenValue().
Comment on attachment 156591 [details] Patch Attachment 156591 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13433848 New failing tests: fast/forms/range/slider-delete-while-dragging-thumb.html fast/loader/stateobjects/state-attribute-object-types.html fast/loader/loadInProgress.html fast/loader/stateobjects/pushstate-object-types.html http/tests/inspector/extensions-network-redirect.html inspector/extensions/extensions-reload.html inspector/extensions/extensions-audits-api.html fast/canvas/webgl/shader-precision-format.html http/tests/inspector/extensions-headers.html http/tests/inspector/extensions-useragent.html http/tests/xmlhttprequest/zero-length-response.html http/tests/security/script-crossorigin-loads-correctly.html inspector/extensions/extensions-console.html inspector/extensions/extensions-network.html inspector/extensions/extensions-audits-content-script.html inspector/extensions/extensions-panel.html fast/forms/range/slider-mouse-events.html fast/frames/cached-frame-counter.html inspector/extensions/extensions-eval-content-script.html inspector/extensions/extensions-audits.html fast/dom/Window/window-postmessage-clone.html fast/dom/Window/window-postmessage-arrays.html inspector/extensions/extensions-resources.html fast/loader/unload-form-post-about-blank.html inspector/extensions/extensions-eval.html fast/forms/range/slider-onchange-event.html inspector/extensions/extensions-api.html
Created attachment 156610 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 156591 [details] Patch hmm, v8::Undefined() should be equivalent to v8::Handle<v8::Value>(), but it looks like that's not true. Let me investigate it.
Created attachment 156619 [details] Patch
Comment on attachment 156619 [details] Patch Attachment 156619 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13445235 New failing tests: plugins/npruntime/npruntime.html
Created attachment 156633 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 156619 [details] Patch I discussed with the V8 team and found the following facts: - v8::Handle<v8::Value>() and v8::Undefined() are not equivalent. A bunch of V8 APIs do not support v8::Handle<v8::Value>(). If we pass v8::Handle<v8::Value>() to such V8 APIs, they crash. We have to pass v8::Undefined(). In other words, we cannot simply replace v8::Undefined() with v8::Handle<v8::Value>(). - For return values of DOM attribute/method callbacks, v8::Handle<v8::Value>() and v8::Undefined() are equivalent. Possible solutions: [A] Implement v8FastUndefined() as follows: v8FastUndefined() { return v8::Handle<v8::Value>(); } Use v8FastUndefined() at the places where v8::Handle<v8::Value>() and v8::Undefined() are equivalent. Use v8::Undefined() otherwise. [B] Implement v8Undefined(Isolate*) as follows: v8Undefined(Isolate* isolate = 0) { return isolate ? v8::Undefined(isolate) : v8::Undefined(); } Use v8Undefined(Isolate*) everywhere. In terms of performance, if an Isolate exists, [A] and [B] are almost the same. If an Isolate does not exist, [A] is faster than [B]. (See https://bugs.webkit.org/show_bug.cgi?id=93093#c0) Which would be better?
> v8::Handle<v8::Value>() and v8::Undefined() are not equivalent. What's the difference? Are the differences bugs in V8?
(In reply to comment #13) > > v8::Handle<v8::Value>() and v8::Undefined() are not equivalent. > > What's the difference? Are the differences bugs in V8? v8::Handle<v8::Value>() is an empty handle. v8::Undefined() is a non-empty handle pointing to an undefined value. v8::Handle<v8::Value> value = v8::Handle<v8::Value>(); value.IsEmpty(); // true v8::Handle<v8::Value> value = v8::Undefined(); value.IsEmpty(); // false Most V8 APIs are not assuming that empty handles are passed to them. I can ask the V8 team to support empty handles, but they might not be willing to do it for performance. c.f. https://chromiumcodereview.appspot.com/10825196/diff/1/src/api.cc c.f. http://code.google.com/p/v8/issues/detail?id=2274
Option [B] sounds better. v8FastUndefined is going to mysterious and confusing. Ideally v8::Undefined would be as fast as v8Undefined so we could use it directly. Maybe it will get faster and we can remove v8Undefined later.
(In reply to comment #15) > Option [B] sounds better. v8FastUndefined is going to mysterious and confusing. Ideally v8::Undefined would be as fast as v8Undefined so we could use it directly. Maybe it will get faster and we can remove v8Undefined later. OK, sounds nice.
DOM attributes that return undefined are much slower in V8 than in JSC (https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AlobCOyvTnPKdDg5S0dMdGRGRTRSaW53V1ppVzh6eXc#gid=2) Thanks to the V8 side fix, now the situation becomes as follows: // 7.3 ns static Handle<Value> testAttr1Getter(AccessorInfo& info) { return Handle<Value>(); } // 7.4 ns static Handle<Value> testAttr1Getter(AccessorInfo& info) { return v8::Undefined(info.GetIsolate()); } // 10.5 ns static Handle<Value> testAttr1Getter(AccessorInfo& info) { return v8::Undefined(); } So, I will make the following change: // V8Binding.h already implements this method Handle<Value> v8Undefined() { return Handle<Value>(); } I will replace all v8::Undefined(isolate) and v8::Undefined() with v8Undefined(). In almost all cases, this replacement works fine. However, in some cases the replacement won't work because Handle<Value>() is different from v8::Undefined(). Handle<Value>() is an empty value whereas v8::Undefined() is an undefined value. We have to use v8::Undefined() at the places where the difference is important.