1295563 - Assert that HTMLEditor can never be reinitialized

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

See bug 1156062 comment 106.

Comment 1

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

So we are definitely sometimes reinitializing an HTML editor:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c65ff0761d&exclusion_profile=false

I haven't looked at the stacks yet.

Comment 2

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

Ehsan, it looks like an HTMLEditor can be reinitialized:

https://treeherder.mozilla.org/logviewer.html#?job_id=25877622&repo=try#L9026

This is contrary to your statement in bug 1156062 comment 106, I think.  In this case it looks like no editing operations are in progress, but it's not obvious to me that that must be the case.  Do you know of an assert that will check if editing operations are in progress?  Or is there something else I'm doing wrong?

As a reminder, the concern here is that HTMLEditRules methods hold a kungFuDeathGrip(mHTMLEditor) and then use mHTMLEditor without worrying that it might have been nulled in the meantime by DetachEditor.

Comment 3

[(no longer active)]

Oh, right.  This is caused by us reusing the editor here: <http://searchfox.org/mozilla-central/source/editor/composer/nsEditingSession.cpp#420> which is primarily there so that we reuse the editor across reframing an iframe and such.

This is a bit worrying, we should probably add the assertion that you're suggesting.

Comment 4

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

Attachment: 0001-Bug-1295563-Assert-that-editors-can-t-be-reinitializ.patch

Ehsan, can you confirm that this assertion is what we want?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc82edf14d2&exclusion_profile=false

Comment 5

[(no longer active)]

Comment on attachment 8783484 [details] [diff] [review]
0001-Bug-1295563-Assert-that-editors-can-t-be-reinitializ.patch

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

Yep!

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8783484 [details] [diff] [review]
0001-Bug-1295563-Assert-that-editors-can-t-be-reinitializ.patch

> NS_IMETHODIMP
> EditorBase::Init(nsIDOMDocument* aDoc,
>                  nsIContent* aRoot,
>                  nsISelectionController* aSelCon,
>                  uint32_t aFlags,
>                  const nsAString& aValue)
> {
>+  // Initializing during an edit action is an error
>+  MOZ_ASSERT(mAction == EditAction::none);

Okay, let's try it. But if you think that you need to add the comment, you can move it into the second argument of MOZ_ASSERT(). I like it's better than separated comment from MOZ_ASSERT() because somebody could separate them accidentally at adding another MOZ_ASSERTION() or modifying here.

>   NS_PRECONDITION(aDoc, "bad arg");

If you don't mind, I'd like you to change this NS_PRECONDITION to MOZ_ASSERT too (I don't think we don't need the useless comment "bad arg" here).

Comment 7

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> If you don't mind, I'd like you to change this NS_PRECONDITION to MOZ_ASSERT
> too (I don't think we don't need the useless comment "bad arg" here).

IIRC, NS_ASSERTION and NS_PRECONDITION require a second argument.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Aryeh Gregor (working until September 2) from comment #7)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> > If you don't mind, I'd like you to change this NS_PRECONDITION to MOZ_ASSERT
> > too (I don't think we don't need the useless comment "bad arg" here).
> 
> IIRC, NS_ASSERTION and NS_PRECONDITION require a second argument.

Yes. I meant that we can omit the comment with replacing NS_PRECONDITION with MOZ_ASSERT.
# Crashing in debug build is better than using NS_ASSERTIOn/NS_PRECONDITION for detecting new regressions

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e8e8f05c92
Assert that editors can't be reinitialized during an edit action; r=masayuki

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47e8e8f05c92