Bug 93912

Summary: Implement UndoManager's V8 bindings
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: HTML EditingAssignee: Sukolsak Sakshuwong <sukolsak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarcelo, dglazkov, gyuyoung.kim, haraken, japhet, mifenton, ojan, rakuco, rniwa, sukolsak, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 89722    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch + remove all JSC bindings
none
Archive of layout-test-results from gce-cr-linux-08
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sukolsak Sakshuwong 2012-08-13 17:18:50 PDT
Implement UndoManager's V8 bindings
Comment 1 Sukolsak Sakshuwong 2012-08-13 17:26:05 PDT
Created attachment 158153 [details]
Patch
Comment 2 Kentaro Hara 2012-08-13 18:19:01 PDT
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()
Comment 3 Sukolsak Sakshuwong 2012-08-13 20:56:54 PDT
Created attachment 158199 [details]
Patch + remove all JSC bindings
Comment 4 Adam Barth 2012-08-13 21:26:04 PDT
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?
Comment 5 Kentaro Hara 2012-08-13 21:48:04 PDT
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()
Comment 6 WebKit Review Bot 2012-08-13 21:57:04 PDT
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
Comment 7 WebKit Review Bot 2012-08-13 21:57:10 PDT
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
Comment 8 Sukolsak Sakshuwong 2012-08-14 05:56:43 PDT
Created attachment 158308 [details]
Patch
Comment 9 Adam Barth 2012-08-15 13:27:39 PDT
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?
Comment 10 Adam Barth 2012-08-15 13:32:15 PDT
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).
Comment 11 Adam Barth 2012-08-15 13:33:15 PDT
I'm still concerned that this patch has use-after-free vulnerabilities.
Comment 12 Sukolsak Sakshuwong 2012-08-15 16:34:00 PDT
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.
Comment 13 Adam Barth 2012-08-15 16:35:58 PDT
> > 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.)
Comment 14 Kentaro Hara 2012-08-15 16:42:44 PDT
(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.
Comment 15 Ryosuke Niwa 2012-08-16 12:43:06 PDT
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.
Comment 16 Sukolsak Sakshuwong 2012-08-16 19:45:18 PDT
Created attachment 158982 [details]
Patch
Comment 17 Kentaro Hara 2012-08-16 20:34:28 PDT
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.
Comment 18 Ryosuke Niwa 2012-08-16 21:45:30 PDT
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.
Comment 19 Sukolsak Sakshuwong 2012-08-16 23:26:31 PDT
Created attachment 159010 [details]
Patch
Comment 20 Ryosuke Niwa 2012-08-16 23:35:52 PDT
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 -.
Comment 21 Sukolsak Sakshuwong 2012-08-16 23:43:13 PDT
Created attachment 159012 [details]
Patch
Comment 22 WebKit Review Bot 2012-08-17 01:08:22 PDT
Comment on attachment 159012 [details]
Patch

Clearing flags on attachment: 159012

Committed r125865: <http://trac.webkit.org/changeset/125865>
Comment 23 WebKit Review Bot 2012-08-17 01:08:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Erik Arvidsson 2012-08-17 07:29:00 PDT
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?
Comment 25 Ryosuke Niwa 2012-08-17 07:31:46 PDT
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 :(