Last Comment Bug 765595 - Make nsEditor::DeleteNode infallible
: Make nsEditor::DeleteNode infallible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Makoto Kato [:m_kato]
Mentors:
Depends on: 771435 773262
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-17 11:10 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2012-07-12 07:56 PDT (History)
2 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1 -- Clean up nsEditor::DoTransaction (6.01 KB, patch)
2012-07-02 08:46 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2 -- De-COMtaminate nsEditor::mTxnMgr (4.20 KB, patch)
2012-07-02 08:46 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch part 3 -- Make PeekUndoStack/PeekRedoStack infallible (37.52 KB, patch)
2012-07-02 08:49 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr (6.74 KB, patch)
2012-07-03 04:09 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v2 -- Make PeekUndoStack/PeekRedoStack infallible (38.56 KB, patch)
2012-07-03 04:10 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch part 4 -- Make nsEditor::DoAfter*Transaction infallible (4.71 KB, patch)
2012-07-03 04:12 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v3 -- Make PeekUndoStack/PeekRedoStack infallible (16.40 KB, patch)
2012-07-05 01:18 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-17 11:10:17 PDT
There's no good reason this should fail, as long as the node is editable.  But it's called an awful lot!
Comment 1 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-17 11:26:55 PDT
The interesting question seems to be: how should we handle the case where the node given to DeleteElementTxn isn't editable?  Currently, DeleteElementTxn::Init returns an error.  But propagating errors like this blindly back up through the call chain is exactly what we want to avoid here.  Really, if any editing code is trying to remove a non-editable node, that's a bug -- callers should be checking for this and handling it appropriately.  (But there are a lot of callers . . .)
Comment 2 :Ehsan Akhgari 2012-06-18 12:58:29 PDT
One possible solution is to temporarily rename the method so that all of the callers fail to compile, and then check for editability in all of them, and then add a MOZ_ASSERT to the implementation of DeleteNode to catch future bad callers.
Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-19 05:52:37 PDT
Using MOZ_ASSERT here makes me nervous, because our test coverage for mixed-editability content of the sort that would trigger it is very poor.  I would expect new code to hit it by mistake in all kinds of cases.  People apparently treat even harmless MOZ_ASSERTs as big deals, because they look like crashes, so perhaps NS_ASSERTION would be better.
Comment 4 :Ehsan Akhgari 2012-06-19 08:20:19 PDT
Hmm, yeah..  Honestly, given the quality of code elsewhere in editor/, I don't see much point in fixing this, since we have a ton of similar cases here and there, and no central way to perform these kinds of checks.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-19 08:40:37 PDT
The idea would have been to try reducing the amount of NS_ENSURE_SUCCESS non-error-handling going around.  I guess this isn't a good candidate, because there is a legitimate failure case here.  I did write a bunch of cleanup patches to try getting close to the goal here before I realized there was a legitimate reason for it to fail, so I'll submit those when I have the time.

What I really want is for the only error return codes to be for sane, easily-understood reasons like "node was not editable", not "some function three levels down the call stack returned an error due to some implementation detail and it was mindlessly propagated up the stack".  So I think it would still be worthwhile to ensure that DeleteNode *only* returns an error if the node was not editable, or possibly in a very limited number of other cases that could be individually enumerated in documentation.  I think this is probably achievable.
Comment 6 :Ehsan Akhgari 2012-06-19 11:40:28 PDT
(In reply to Aryeh Gregor from comment #5)
> What I really want is for the only error return codes to be for sane,
> easily-understood reasons like "node was not editable", not "some function
> three levels down the call stack returned an error due to some
> implementation detail and it was mindlessly propagated up the stack".  So I
> think it would still be worthwhile to ensure that DeleteNode *only* returns
> an error if the node was not editable, or possibly in a very limited number
> of other cases that could be individually enumerated in documentation.  I
> think this is probably achievable.

Yeah that's a good idea.
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-02 08:46:06 PDT
Created attachment 638378 [details] [diff] [review]
Patch part 1 -- Clean up nsEditor::DoTransaction
Comment 8 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-02 08:46:29 PDT
Created attachment 638379 [details] [diff] [review]
Patch part 2 -- De-COMtaminate nsEditor::mTxnMgr
Comment 9 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-02 08:49:29 PDT
Created attachment 638380 [details] [diff] [review]
Patch part 3 -- Make PeekUndoStack/PeekRedoStack infallible

This removes a failure path from nsEditor::DoTransaction, which is called by DeleteNode, so it brings us a step closer to our goal.  I had these patches sitting around, so I figured I'd submit them to avoid bitrot.  They apply on top of bug 769967.

Try: https://tbpl.mozilla.org/?tree=Try&rev=e9b45308a37d
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-03 02:57:17 PDT
Comment on attachment 638378 [details] [diff] [review]
Patch part 1 -- Clean up nsEditor::DoTransaction

Bah, build failure.  Will fix and re-request review.
Comment 11 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-03 04:09:44 PDT
Created attachment 638645 [details] [diff] [review]
Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr

New!  Improved!  Compiles on localhost!
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-03 04:10:31 PDT
Created attachment 638646 [details] [diff] [review]
Patch part 3, v2 -- Make PeekUndoStack/PeekRedoStack infallible

Also now with the benefit of actually compiling.
Comment 13 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-03 04:12:41 PDT
Created attachment 638647 [details] [diff] [review]
Patch part 4 -- Make nsEditor::DoAfter*Transaction infallible

I think now the only ways for DeleteNode to fail are 1) DeleteNodeTxn::Init fails due to non-editable node, 2) DeleteNodeTxn::DoTransaction fails due to the node having been removed from its parent in the meantime, 3) GetSelection returns null.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3ff3fd0fc5f8
Comment 14 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-03 07:01:45 PDT
Comment on attachment 638645 [details] [diff] [review]
Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr

(In reply to :Aryeh Gregor from comment #11)
> Created attachment 638645 [details] [diff] [review]
> Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr
> 
> New!  Improved!  Compiles on localhost!

But not on non-Linux platforms, because they apparently didn't include mozilla/Assertions.h when Linux did for some reason.  Sigh.  Plus there are test failures on compile.  So I think I'm going to back off on these patches until I get a green try run.
Comment 15 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-05 01:18:08 PDT
Created attachment 639256 [details] [diff] [review]
Patch part 3, v3 -- Make PeekUndoStack/PeekRedoStack infallible

Now with (hopefully) correct XPCOM usage and header includes.  And most importantly, a green try run: https://tbpl.mozilla.org/?tree=Try&rev=d0d7de7786d9
Comment 16 :Ehsan Akhgari 2012-07-05 09:02:16 PDT
Comment on attachment 638378 [details] [diff] [review]
Patch part 1 -- Clean up nsEditor::DoTransaction

Review of attachment 638378 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +628,3 @@
>      nsCOMPtr<nsIAbsorbingTransaction> plcTxn;
> +    editTxn->QueryInterface(NS_GET_IID(nsIAbsorbingTransaction),
> +                            getter_AddRefs(plcTxn));

Can you please just use do_QueryInterface here?
Comment 17 :Ehsan Akhgari 2012-07-05 09:08:17 PDT
Comment on attachment 638645 [details] [diff] [review]
Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr

Review of attachment 638645 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +752,5 @@
>  nsEditor::SetTransactionManager(nsITransactionManager *aTxnManager)
>  {
>    NS_ENSURE_TRUE(aTxnManager, NS_ERROR_FAILURE);
>  
> +  mTxnMgr = static_cast<nsTransactionManager*>(aTxnManager);

Please add a comment saying that nsITransactionManager is builtinclass.

::: editor/txmgr/public/Makefile.in
@@ +17,2 @@
>  		nsTransactionManagerCID.h \
> +		nsTransactionStack.h \

Instead of exporting these headers, please adjust the LOCAL_INCLUDES for editor/libeditor/base to point to this directory.
Comment 18 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-06 00:59:43 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +628,3 @@
> >      nsCOMPtr<nsIAbsorbingTransaction> plcTxn;
> > +    editTxn->QueryInterface(NS_GET_IID(nsIAbsorbingTransaction),
> > +                            getter_AddRefs(plcTxn));
> 
> Can you please just use do_QueryInterface here?

Originally the code had to do this because it used a factory function that returned EditTxn, but bug 489851 fixed that, so really it's a PlaceholderTxn.  But that inherits multiply from nsISupports, so do_GetWeakReference doesn't work on it.  So I couldn't figure out a configuration here that will compile -- however it works, it will probably need some way to say which class to derive from.

I'm happy to fix this in a follow-up if you tell me how to do it.  Filed bug 771435.

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Instead of exporting these headers, please adjust the LOCAL_INCLUDES for
> editor/libeditor/base to point to this directory.

Okay.  I was told conflicting things in #developers, so I went with Ms2ger and just exported them.  I wound up having to add it to a bunch of makefiles: editor/libeditor/base, editor/libeditor/text, editor/libeditor/html, content/html/content/src, layout/build, and layout/forms, because all of them included nsEditor.h one way or another.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a05040374df3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31578d20f5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bdb22e380c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/05f4bcada6b9

Note You need to log in before you can comment on or make changes to this bug.