[Webkit-unassigned] [Bug 93912] Implement UndoManager's V8 bindings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93912





--- Comment #12 from Sukolsak Sakshuwong <sukolsak at google.com>  2012-08-15 16:34:00 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.