765595 - Make nsEditor::DeleteNode infallible

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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) (no longer with Mozilla)]

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

[(no longer active)]

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) (no longer with Mozilla)]

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

[(no longer active)]

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) (no longer with Mozilla)]

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

[(no longer active)]

(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) (no longer with Mozilla)]

Attachment: Patch part 1 -- Clean up nsEditor::DoTransaction

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- De-COMtaminate nsEditor::mTxnMgr

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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) (no longer with Mozilla)]

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) (no longer with Mozilla)]

Attachment: Patch part 2, v2 -- De-COMtaminate nsEditor::mTxnMgr

New!  Improved!  Compiles on localhost!

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v2 -- Make PeekUndoStack/PeekRedoStack infallible

Also now with the benefit of actually compiling.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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) (no longer with Mozilla)]

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) (no longer with Mozilla)]

Attachment: 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

[(no longer active)]

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

[(no longer active)]

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) (no longer with Mozilla)]

(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

Comment 19

[(no longer active)]

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