Bug 93218 – [V8] Replace v8::Undefined() in bindings/v8/* with v8Undefined()

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 156583 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 156591 [details]
Patch

[Kentaro Hara]

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().

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 156619 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

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?

[Adam Barth]

> v8::Handle<v8::Value>() and v8::Undefined() are not equivalent.

What's the difference?  Are the differences bugs in V8?

[Kentaro Hara]

(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

[Adam Barth]

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.

[Kentaro Hara]

(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.

[Kentaro Hara]

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.