Bug 93912 – Implement UndoManager's V8 bindings

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sukolsak Sakshuwong]

Implement UndoManager's V8 bindings

[Sukolsak Sakshuwong]

Created attachment 158153 [details]
Patch

[Kentaro Hara]

Comment on attachment 158153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158153&action=review

Added some comments.

Actually I'm not so excited at this patch --- the custom implementations are too dirty.

- If undo, redo, etc were speced as EventListeners, the implementation would become much simpler.

- If you really want to implement undo, redo, etc by the "user object" in the Web IDL spec, we might want to implement the general mechanism of the "user object" to CodeGenerators, instead of adding ad-hoc implementations to custom bindings. But for now I'm not sure how we can generally implement the "user object" in CodeGenerators.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

Use the copyright in WebKit/Source/WebKit/LICENSE

> Source/WebCore/bindings/v8/DOMTransaction.cpp:43
> +    : m_worldContext(worldContext)

I don't fully understand the lifetime of the DOM transaction and the context. Is it guaranteed that m_worldContext is alive while the DOM transaction is alive?

> Source/WebCore/bindings/v8/DOMTransaction.cpp:87
> +    return !function.IsEmpty() && function->IsFunction();

Nit: I would prefer !(function.IsEmpty() || !function->IsFunction()) for consistency.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:112
> +    v8::HandleScope handleScope;
> +
> +    v8::Local<v8::Context> v8Context = toV8Context(context, m_worldContext);
> +    if (v8Context.IsEmpty())
> +        return;
> +
> +    v8::Context::Scope scope(v8Context);
> +

I don't fully understand the code around here. Just in case, would you elaborate on why you need to create a new Context and Scope?

> Source/WebCore/bindings/v8/DOMTransaction.h:64
> +    bool getFunction(const char* propertyName, v8::Local<v8::Value>& function);
> +    bool hasFunction(const char* propertyName);
> +    void callFunction(const char* propertyName);

Nit: variable names are not needed.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:60
> +    transactionWrapper->SetHiddenValue(v8::String::New(DOMTRANSACTION_DATA), dictionary);

v8::String::New() => v8::String::NewSymbol()

[Sukolsak Sakshuwong]

Created attachment 158199 [details]
Patch + remove all JSC bindings

[Adam Barth]

Comment on attachment 158199 [details]
Patch + remove all JSC bindings

View in context: https://bugs.webkit.org/attachment.cgi?id=158199&action=review

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:54
> +    transactionWrapper->SetHiddenValue(v8::String::NewSymbol(DOMTRANSACTION_DATA), dictionary);

Should we use V8HiddenPropertyName to keep track of the name?

[Kentaro Hara]

Comment on attachment 158199 [details]
Patch + remove all JSC bindings

View in context: https://bugs.webkit.org/attachment.cgi?id=158199&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:74
> +    v8::Handle<v8::Object> wrapper = v8::Handle<v8::Object>::Cast(toV8(this));

wrapper.IsEmpty() check would be needed.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:42
> +        return V8Proxy::throwNotEnoughArgumentsError(args.GetIsolate());

Nit: Now V8Proxy:: is not needed. Please rebase with the latest WebKit trunk.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:47
> +        return V8Proxy::throwTypeError("The first argument is not of type DOMTransaction.", args.GetIsolate());

Nit: Ditto.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:59
> +        return V8Proxy::setDOMException(ec, args.GetIsolate());

Nit: Ditto.

> Source/WebCore/bindings/v8/custom/V8UndoManagerCustom.cpp:60
> +    return v8::Handle<v8::Value>();

v8Undefined()

[WebKit Review Bot]

Comment on attachment 158199 [details]
Patch + remove all JSC bindings

Attachment 158199 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13503021

New failing tests:
http/tests/cache/post-redirect-get.php
http/tests/cache/post-with-cached-subresources.php

[WebKit Review Bot]

Created attachment 158208 [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

[Sukolsak Sakshuwong]

Created attachment 158308 [details]
Patch

[Adam Barth]

Comment on attachment 158308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review

> Source/WebCore/bindings/v8/DOMTransaction.cpp:61
> +    if (m_undoManager)
> +        m_undoManager->registerRedoStep(this);

Isn't this a use-after-free?  What stops the undo function from destroying |this|.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:84
> +    return !(function.IsEmpty() || !function->IsFunction());

So many negatives!  How about:

return !function.IsEmpty() && function->IsFunction();

> Source/WebCore/bindings/v8/DOMTransaction.cpp:99
> +    if (!context || context->isJSExecutionForbidden())

isJSExecutionForbidden only applies for workers.  In this function only runs on the main thread, so it's not needed.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:102
> +    Frame* frame = static_cast<Document*>(context)->frame();

Why does |context| have the type ScriptExecutionContext if it's always a Document*?  It seems more straightforward to just have

Document* document = m_undoManager->undoScopeHost()->document();

above.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:110
> +    v8::Local<v8::Function> function = v8::Local<v8::Function>::Cast(property);

It seems like getFunction should just return a v8::Handle<v8::Function> and return an empty handle when it can't find one rather than having both a Boolean return value and an out parameter of the wrong type.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:118
> +    frame->script()->proxy()->callFunction(function, receiver, 0, parameters);

We're supposed to call the function with itself as the receiver?  That's a bit odd.  I would have expected the UndoManager or the global object to be the receiver.  Is there a test for this?

> Source/WebCore/bindings/v8/custom/V8DOMTransactionCustom.cpp:41
> +    v8::Persistent<v8::Object> ownerWrapper = store->domObjectMap().get(undoManager);

ownerWrapper -> undoManagerWrapper?

[Adam Barth]

Comment on attachment 158308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:17
> +var u = document.undoManager;

u -> please don't use single letter variable names.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:32
> +	if (window.GCController)
> +		GCController.collect();

If you include ../../resources/gc.js you can call gc() in the global scope, which will work both in DumpRenderTree and in browsers.

> LayoutTests/editing/undomanager/undomanager-transact.html:36
> +        "execute": function() {
> +            edit.innerHTML += "1e ";
> +        },
> +        "undo": function() {
> +            edit.innerHTML += "1u ";
> +        },
> +        "redo": function() {
> +            edit.innerHTML += "1r ";
> +        }

Can you test what the |this| value is during these callbacks?

Also, please add tests for the use-after-free issues and please also test what happens when we re-enter UndoManager by calling undoManager functions during callbacks.

It would also be good to test the isolated world interactions via testRunner.executeScriptInIsolatedWorld (I'm not sure if that name is 100% accurate).

[Adam Barth]

I'm still concerned that this patch has use-after-free vulnerabilities.

[Sukolsak Sakshuwong]

Thank you for the comments.

(In reply to comment #9)
> (From update of attachment 158308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review
> 
> > Source/WebCore/bindings/v8/DOMTransaction.cpp:61
> > +    if (m_undoManager)
> > +        m_undoManager->registerRedoStep(this);
> 
> Isn't this a use-after-free?  What stops the undo function from destroying |this|.

UndoManager::undo() and redo() are the only methods that call DOMTransaction::unapply() and reapply(). Since UndoManager is guaranteed to be alive during those calls (because of "protect(this)") and it has a strong reference to the DOMTransaction (in the local copy of the undo/redo stack), |this| should not be destroyed.

> > Source/WebCore/bindings/v8/DOMTransaction.cpp:84
> > +    return !(function.IsEmpty() || !function->IsFunction());
> 
> So many negatives!  How about:
> 
> return !function.IsEmpty() && function->IsFunction();

I previously used that but changed it to the current code as suggested by haraken's comment #2.
 
> Can you test what the |this| value is during these callbacks?
> 
> Also, please add tests for the use-after-free issues and please also test what happens when we re-enter UndoManager by calling undoManager functions during callbacks.
> 
> It would also be good to test the isolated world interactions via testRunner.executeScriptInIsolatedWorld (I'm not sure if that name is 100% accurate).

I will do.

[Adam Barth]

> > Source/WebCore/bindings/v8/DOMTransaction.cpp:87
> > +    return !function.IsEmpty() && function->IsFunction();
> 
> Nit: I would prefer !(function.IsEmpty() || !function->IsFunction()) for consistency.

Consistency with what?  (I don't mind either way---I'm just curious.)

[Kentaro Hara]

(In reply to comment #13)
> > > Source/WebCore/bindings/v8/DOMTransaction.cpp:87
> > > +    return !function.IsEmpty() && function->IsFunction();
> > 
> > Nit: I would prefer !(function.IsEmpty() || !function->IsFunction()) for consistency.
> 
> Consistency with what?  (I don't mind either way---I'm just curious.)

'value.IsEmpty() || !value.IsXXX()' is a typical pattern for the type check (also used in this patch). I don't mind either way. Fewer negatives might be better.

[Ryosuke Niwa]

Comment on attachment 158308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158308&action=review

> Source/WebCore/ChangeLog:7
> +

You should describe what kind of change you're making here.

> Source/WebCore/bindings/js/DOMTransaction.h:46
> +    EditAction editingAction() const OVERRIDE { return EditActionUnspecified; }
> +    bool isDOMTransaction() const OVERRIDE { return true; }

Missing virtual.

> Source/WebCore/bindings/v8/DOMTransaction.cpp:59
> +

Assert that m_undoManager->refCount() is non-zero here so as to address Adam's concern?

>>> Source/WebCore/bindings/v8/DOMTransaction.cpp:84
>>> +    return !(function.IsEmpty() || !function->IsFunction());
>> 
>> So many negatives!  How about:
>> 
>> return !function.IsEmpty() && function->IsFunction();
> 
> I previously used that but changed it to the current code as suggested by haraken's comment #2.

Maybe you can create a local variable to make the semantic clear.

> Source/WebCore/bindings/v8/DOMTransaction.h:47
> +    EditAction editingAction() const OVERRIDE { return EditActionUnspecified; }
> +    bool isDOMTransaction() const OVERRIDE { return true; }

Missing virtual. I'm surprised it works.

> Source/WebCore/editing/UndoManager.cpp:61
> +void UndoManager::stop()

You can define stop and clearStack after transact to make the diff look saner.

> Source/WebCore/editing/UndoManager.cpp:91
> +    if (m_isInProgress || !isConnected()) {

Eventually, this m_inInProgress flag needs to be global because we disallow execution of any transaction while we're applying, unapplying, or reapplying a transaction anywhere.
This can be done a separate patch of course.

> Source/WebCore/editing/UndoManager.cpp:104
> +        m_undoStack.append(adoptPtr(new UndoManagerEntry));

Please add static create function to UndoManagerEntry and adoptPtr there.

> Source/WebCore/editing/UndoManager.cpp:116
> +    m_inProgressEntry = adoptPtr(new UndoManagerEntry);

Ditto.

> Source/WebCore/editing/UndoManager.cpp:118
> +    m_isInProgress = true;

Once we make isInProgress global, we can just turn this into a RAII.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:21
> +	"execute": function() {
> +		log.innerText += ("execute\n");
> +	},

You can put the whole thing in one line.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:40
> +	if (window.GCController)
> +		GCController.collect();

I don't think we should be using testharness.js in the test that uses GCController since we can't export to W3C anyway.
Also, there's a tab character here.

[Sukolsak Sakshuwong]

Created attachment 158982 [details]
Patch

[Kentaro Hara]

Comment on attachment 158982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158982&action=review

LGTM for V8 bindings, empty JSC bindings, and IDL files.

rniwa: would you take a look at the WebCore part and tests?

> Source/WebCore/bindings/v8/DOMTransaction.h:34
> +#define DOMTRANSACTION_DATA "data"

Nit: This is no longer used.

[Ryosuke Niwa]

Comment on attachment 158982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158982&action=review

> Source/WebCore/editing/UndoManager.cpp:62
> +        if (entry[0]->isDOMTransaction()) {

Isn't it possible for someone to execute execCommand as is?
which means that we still have to check isDOMTransaction for each item in the entry, no?

> Source/WebCore/editing/UndoManager.cpp:174
> +void UndoManager::undo()
> +{
> +    undo(ASSERT_NO_EXCEPTION);
> +}
> +
> +void UndoManager::redo()
> +{
> +    redo(ASSERT_NO_EXCEPTION);
> +}

You can just use ASSERT_NO_EXCEPTION as the default argument value.

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:21
> +    "execute": function() { result += ("execute "); },

Why do we need parenthesis around "execute "?

> LayoutTests/editing/undomanager/domtransaction-survives-gc.html:31
> +if (result == "execute undo ")
> +    log.innerHTML += "PASS: after the GC ran, undoManager.undo() still worked.<br>";
> +else
> +    log.innerHTML += "FAIL: after the GC ran, undoManager.undo() did not work.<br>";

You can include js-test-pre and use testPassed / testFailed.
Better yet, shouldBe("gc(); document.undoManager.undo(); result", "execute undo ");

> LayoutTests/editing/undomanager/undomanager-isolated-world.html:13
> +if (window.testRunner) {
> +    testRunner.dumpAsText();

We typically output an error message like "this test requires testRunner" when the test requires testRunner and cannot be ran manually.

> LayoutTests/editing/undomanager/undomanager-isolated-world.html:30
> +    var log = document.getElementById('log');
> +    if (log.innerHTML == "")
> +        log.innerHTML = "PASS: undoManager's callback was not invoked in isolated world."

Ditto about using js-test-pre.js.

[Sukolsak Sakshuwong]

Created attachment 159010 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159010 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159010&action=review

> Source/WebCore/editing/UndoManager.cpp:59
> +        for (size_t j = 0; j < entry.size()-1; ++j) {

Nit: Need a space around -.

[Sukolsak Sakshuwong]

Created attachment 159012 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159012 [details]
Patch

Clearing flags on attachment: 159012

Committed r125865: <http://trac.webkit.org/changeset/125865>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

Comment on attachment 159012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159012&action=review

> Source/WebCore/editing/DOMTransaction.idl:29
> +        V8CustomIsReachable

Should this have an interfaceName=Transaction?

[Ryosuke Niwa]

Comment on attachment 159012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159012&action=review

>> Source/WebCore/editing/DOMTransaction.idl:29
>> +        V8CustomIsReachable
> 
> Should this have an interfaceName=Transaction?

No, the interface name in the spec is "DOMTransaction". Perhaps this is confusing :(