Assert that HTMLEditor can never be reinitialized

RESOLVED FIXED in Firefox 51

Status

()

Core
Editor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

See bug 1156062 comment 106.
Summary: Assert that nsHTMLEditor can never be reinitialized → Assert that HTMLEditor can never be reinitialized
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.
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.
Flags: needinfo?(ehsan)

Comment 3

a year ago
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.
Flags: needinfo?(ehsan)
Created attachment 8783484 [details] [diff] [review]
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
Attachment #8783484 - Flags: review?(masayuki)
Attachment #8783484 - Flags: feedback?(ehsan)

Comment 5

a year ago
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!
Attachment #8783484 - Flags: feedback?(ehsan) → feedback+
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).
Attachment #8783484 - Flags: review?(masayuki) → review+
(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.
(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

a year ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e8e8f05c92
Assert that editors can't be reinitialized during an edit action; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/47e8e8f05c92
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.