1054735 - Clean up nsAutoReplaceContainerSelNotify; nsEditor::RemoveContainer, InsertContainerAbove

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

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

Attachment: Part 1 -- Clean up nsAutoReplaceContainerSelNotify

Notice that I put it in mozilla::dom to avoid having to use "dom::Element".  It seems reasonable to me to put new editor things in mozilla::dom, since we use dom so heavily, but let me know if just "mozilla" is better.

Actually, while I'm doing this, I guess it makes sense to remove the "ns", right?

Comment 2

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

Attachment: Part 2 -- Clean up nsEditor::RemoveContainer

Based on the name of this function, I considered making it take an Element* instead of nsIContent*, but figured some callers might be calling it on things that might or might not be containers.

As a general note, my approach these days is not to overload functions to take both old-style and new-style nodes, but rather to just change all the callers to use new-style nodes.  Otherwise you have to go back later and check that all the callers of the old version have actually been removed, which may take a really long time.  This way I can actually purge entire files of nsIDOMNode* (excepting NS_IMETHOD stuff).

Comment 3

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

Attachment: Part 3 -- Clean up nsEditor::InsertContainerAbove

What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it okay to remove these checks, or are they actually necessary?  If something is really going to null mHTMLEditor from under us, should we just take a reference in a local nsCOMPtr?

Also, is there any reason to have nsEditProperty, or is it better to just use nsGkAtoms?  I've been assuming the latter.  If that's right, should I just kill nsEditProperty?

Comment 4

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

Attachment: Part 3, v2 -- Clean up nsEditor::InsertContainerAbove

Previous version had a silly bug that got caught by tests.

Comment 5

[(no longer active)]

Comment on attachment 8474213 [details] [diff] [review]
Part 1 -- Clean up nsAutoReplaceContainerSelNotify

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

FWIW putting the names in mozilla::dom and removing the ns prefix in general are good ideas.

::: editor/libeditor/nsSelectionState.h
@@ +212,5 @@
> +                                    Element* aOriginalElement,
> +                                    Element* aNewElement)
> +    : mRU(aRangeUpdater)
> +    , mOriginalElement(aOriginalElement)
> +    , mNewElement(aNewElement)

Nit: please indent the initializer list.

Comment 6

[(no longer active)]

Comment on attachment 8474512 [details] [diff] [review]
Part 3, v2 -- Clean up nsEditor::InsertContainerAbove

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4935,3 @@
>    }
>    if (deepestStyle) {
> +    *aOutBrNode = GetAsDOMNode(CreateBR(deepestStyle, 0).take());

Nit: please use .forget() when forgetting into as nsInterface**.

Comment 7

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Nit: please use .forget() when forgetting into as nsInterface**.

Hmm, I don't understand.  .forget() on what?  CreateBR returns already_AddRefed, and I only see a .forget() method on nsCOMPtr/nsRefPtr, not already_AddRefed.  Do you mean you'd prefer creating an explicit temporary nsCOMPtr instead of .take()?

Comment 8

[(no longer active)]

(In reply to :Aryeh Gregor from comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > Nit: please use .forget() when forgetting into as nsInterface**.
> 
> Hmm, I don't understand.  .forget() on what?  CreateBR returns
> already_AddRefed, and I only see a .forget() method on nsCOMPtr/nsRefPtr,
> not already_AddRefed.  Do you mean you'd prefer creating an explicit
> temporary nsCOMPtr instead of .take()?

Oh hmm, no I was wrong, nevermind.  Sorry!

Comment 9

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

(In reply to :Aryeh Gregor from comment #3)
> What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it
> okay to remove these checks, or are they actually necessary?  If something
> is really going to null mHTMLEditor from under us, should we just take a
> reference in a local nsCOMPtr?
> 
> Also, is there any reason to have nsEditProperty, or is it better to just
> use nsGkAtoms?  I've been assuming the latter.  If that's right, should I
> just kill nsEditProperty?

Comment 10

[(no longer active)]

(In reply to :Aryeh Gregor from comment #9)
> (In reply to :Aryeh Gregor from comment #3)
> > What's up with all the "NS_ENSURE_STATE(mHTMLEditor);" in this file?  Is it
> > okay to remove these checks, or are they actually necessary?  If something
> > is really going to null mHTMLEditor from under us, should we just take a
> > reference in a local nsCOMPtr?

mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That can be called either when we detach an editor from an element (for example during frame reconstruction) or when we destroy the editor (the rules object can technically outlive the editor object!)

Neither of these scenarios are things that we can easily protect against without these types of checks.

> > Also, is there any reason to have nsEditProperty, or is it better to just
> > use nsGkAtoms?  I've been assuming the latter.  If that's right, should I
> > just kill nsEditProperty?

It would be nice if you killed nsEditProperty.  It may have some atoms not in nsGkAtoms, and those we can easily add there.  I _think_ nsEditProperty may predate nsGkAtoms, but there is no good reason for it to exist any more.

Comment 11

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That
> can be called either when we detach an editor from an element (for example
> during frame reconstruction) or when we destroy the editor (the rules object
> can technically outlive the editor object!)
> 
> Neither of these scenarios are things that we can easily protect against
> without these types of checks.

Why can't we keep a reference to the editor in a local nsCOMPtr for the duration of the method's execution?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Comment on attachment 8474213 [details] [diff] [review]
> Part 1 -- Clean up nsAutoReplaceContainerSelNotify
> 
> Review of attachment 8474213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW putting the names in mozilla::dom and removing the ns prefix in general
> are good ideas.

mozilla::dom is for DOM code, though.

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=3ff035fae451

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ffe54b22d8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d121ead4ff7
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe8c5709d09

(In reply to :Ms2ger from comment #12)
> mozilla::dom is for DOM code, though.

editor is DOM code.  :)

Comment 14

[(no longer active)]

(In reply to :Aryeh Gregor from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > mHTMLEditor can be null after a call to nsHTMLEditRules::DetachEditor.  That
> > can be called either when we detach an editor from an element (for example
> > during frame reconstruction) or when we destroy the editor (the rules object
> > can technically outlive the editor object!)
> > 
> > Neither of these scenarios are things that we can easily protect against
> > without these types of checks.
> 
> Why can't we keep a reference to the editor in a local nsCOMPtr for the
> duration of the method's execution?

One danger here is if someone else has already freed the editor by the time we get to run this code.  If that happens, a local nsCOMPtr on the stack won't save us.  The other danger here is if the code in the method destroys the editor, but I'm fairly certain that all of those bugs have been fixed by now.  But yeah, it's probably worth doing what you suggest.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ffe54b22d8a
https://hg.mozilla.org/mozilla-central/rev/8d121ead4ff7
https://hg.mozilla.org/mozilla-central/rev/dbe8c5709d09

Comment 16

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> One danger here is if someone else has already freed the editor by the time
> we get to run this code.  If that happens, a local nsCOMPtr on the stack
> won't save us.  The other danger here is if the code in the method destroys
> the editor, but I'm fairly certain that all of those bugs have been fixed by
> now.  But yeah, it's probably worth doing what you suggest.

How could either of those things happen if the refcount is nonzero?  Aren't refcounted classes supposed to be deleted only by their own Release methods?

Comment 17

[(no longer active)]

(In reply to :Aryeh Gregor from comment #16)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #14)
> > One danger here is if someone else has already freed the editor by the time
> > we get to run this code.  If that happens, a local nsCOMPtr on the stack
> > won't save us.  The other danger here is if the code in the method destroys
> > the editor, but I'm fairly certain that all of those bugs have been fixed by
> > now.  But yeah, it's probably worth doing what you suggest.
> 
> How could either of those things happen if the refcount is nonzero?  Aren't
> refcounted classes supposed to be deleted only by their own Release methods?

On the first case, what I meant was a scenario where you create a transaction object, hand it off an nsIEditor* with non-zero refcount, which is stored as a plain pointer, and _then_ the refcount dropping to zero, killing the editor object, therefore making our weak nsIEditor* pointer a dangling one.

On the second case, the issue is where you have a weak pointer to a refcounted object, you call into some code which ends up dropping the last reference to the object, and by the time that call returns, your pointer would be dangling, but like I said, I don't think that these classes of bugs can happen in the editor code any more.  (But I could be wrong on that, which is why I said it's probably worth doing what you suggest.)