Closed Bug 1457083 Opened 2 years ago Closed Last year

TextEditRules and HTMLEditRules should grab editor instance only once when their public methods are called

Categories

(Core :: Editor, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files)

59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
A lot of methods of TextEditRules and HTMLEditRules grabs editor instance and Selection instance of it by themselves. However, this means that we they may increment those refcounts multiple times per edit action and may cause security issue if they forget to do that or check immediately before every use.

I think that public methods of them should guarantee the lifetime of them during a call and each method should stop grabbing them by themselves.
Priority: -- → P3
Comment on attachment 8976631 [details]
Bug 1457083 - part 1: Make public methods of TextEditUtils and HTMLEditUtils guarantee that the editor instance and selection instance won't be destroyed while it handles any edit actions

https://reviewboard.mozilla.org/r/244716/#review250960
Attachment #8976631 - Flags: review?(m_kato) → review+
Comment on attachment 8976632 [details]
Bug 1457083 - part 2: Make all observer methods of HTMLEditRules create AutoSafeEditorData

https://reviewboard.mozilla.org/r/244718/#review250976
Attachment #8976632 - Flags: review?(m_kato) → review+
Comment on attachment 8976633 [details]
Bug 1457083 - part 3: Replace mTextEditor in TextEditRules with TextEditorRef()

https://reviewboard.mozilla.org/r/244720/#review251282

::: editor/libeditor/TextEditRulesBidi.cpp:41
(Diff revision 2)
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
>    *aCancel = false;
>  
> -  nsCOMPtr<nsIPresShell> shell = mTextEditor->GetPresShell();
> +  nsIPresShell* presShell = TextEditorRef().GetPresShell();

presShell is unused witout getting PresContext, so you should use EditorBase::GetPresContext() directly.
Attachment #8976633 - Flags: review?(m_kato) → review+
Comment on attachment 8976634 [details]
Bug 1457083 - part 4: Replace mHTMLEditor in HTMLEditRules with HTMLEditorRef()

https://reviewboard.mozilla.org/r/244722/#review251306
Attachment #8976634 - Flags: review?(m_kato) → review+
Comment on attachment 8976635 [details]
Bug 1457083 - part 5: Make all users of Selection in TextEditRules and HTMLEditRules use TextEditRules::SelectionRef()

https://reviewboard.mozilla.org/r/244724/#review251308
Attachment #8976635 - Flags: review?(m_kato) → review+
Comment on attachment 8976636 [details]
Bug 1457083 - part 6: Get rid of unnecessary Selection argument from all protected/private methods of TextEditRules and HTMLEditRules

https://reviewboard.mozilla.org/r/244726/#review251312

nits: The parameters (cancel and handled) of Will\* methods uses both assertion (WillInsert etc) and nullptr check (WillInsertBreak etc).   I think that it is better to use MOZ_ASSERT for all Will\* methods.  (It isn't xpcom method, so I don't think no situation to use nullptr for cancel and handled parameters).
Attachment #8976636 - Flags: review?(m_kato) → review+
Comment on attachment 8976637 [details]
Bug 1457083 - part 7: Remove methods of TextEditRules and HTMLEditRules, which just return NS_OK

https://reviewboard.mozilla.org/r/244728/#review251316
Attachment #8976637 - Flags: review?(m_kato) → review+
Comment on attachment 8976636 [details]
Bug 1457083 - part 6: Get rid of unnecessary Selection argument from all protected/private methods of TextEditRules and HTMLEditRules

https://reviewboard.mozilla.org/r/244726/#review251312

I think that we should make them return EditActionResult which has nsresult, canceled and handled state.
https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/editor/libeditor/EditorUtils.h#38
But should be in another bug.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d7e797f898b0
part 1: Make public methods of TextEditUtils and HTMLEditUtils guarantee that the editor instance and selection instance won't be destroyed while it handles any edit actions r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fcacbe739f67
part 2: Make all observer methods of HTMLEditRules create AutoSafeEditorData r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4320b35386e9
part 3: Replace mTextEditor in TextEditRules with TextEditorRef() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c469a8b14bf4
part 4: Replace mHTMLEditor in HTMLEditRules with HTMLEditorRef() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7574aa05f0cf
part 5: Make all users of Selection in TextEditRules and HTMLEditRules use TextEditRules::SelectionRef() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e84f2a4cd2ae
part 6: Get rid of unnecessary Selection argument from all protected/private methods of TextEditRules and HTMLEditRules r=m_kato
https://hg.mozilla.org/integration/autoland/rev/363c21e529d0
part 7: Remove methods of TextEditRules and HTMLEditRules, which just return NS_OK r=m_kato
You need to log in before you can comment on or make changes to this bug.