Implement UndoManager's V8 bindings
Created attachment 158153 [details] Patch
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()
Created attachment 158199 [details] Patch + remove all JSC bindings
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 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 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
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
Created attachment 158308 [details] Patch
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 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).
I'm still concerned that this patch has use-after-free vulnerabilities.
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.
> > 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.)
(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 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.
Created attachment 158982 [details] Patch
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 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.
Created attachment 159010 [details] Patch
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 -.
Created attachment 159012 [details] Patch
Comment on attachment 159012 [details] Patch Clearing flags on attachment: 159012 Committed r125865: <http://trac.webkit.org/changeset/125865>
All reviewed patches have been landed. Closing bug.
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 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 :(