Needs to remove nsAutoScriptBlocker from nsTextEditorState::SetValue()
Categories
(Core :: DOM: Editor, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox72 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
It was introduced by bug 534785:
https://bugzilla.mozilla.org/attachment.cgi?id=447636&action=diff
The comment which was posted with the patch is:
Today I finally managed to reproduce this. The problem was that
IsPreformatted
's call toFlushPendingNotifications
causes thePrepareEditor
event to run, which proceeded to callnsPlaintextEditor::Init
, which caused thensTextEditRules
object on the stack being deleted. The correct fix here is to add a script blocker on the stack innsTextEditorState::SetValue
to make sure that any pendingPrepareEditor
calls are not executed untilInsertText
is fully executed.
Fortunately, we don't have TextEditRules
(nor HTMLEditRules
) anymore (they were merged into the editor classes) and this blocks TextEditor
to dispatch beforeinput
event as cancelable. So, I need to remove the script blocker from it.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
And cleaning up the cpp file.
Assignee | ||
Comment 2•5 years ago
|
||
Some members of TextControlState
are initialized and restored in same block
scopes. Therefore, with creating new stack only class and storing latest one
with a new member variable, we can store all of them in the stack.
Depends on D51391
Assignee | ||
Comment 3•5 years ago
|
||
Currently, nobody guarantees that TextControlState
won't be deleted while
it handles something with MOZ_CAN_RUN_SCRIPT
methods.
This patch hides its destructor (and constructor) for making only
TextControlState
itself can delete its instances. Then, if instance owner
wants to delete it while handling action(s), the oldest AutoHandlingState
will delete the TextControlState
.
Depends on D51392
Assignee | ||
Comment 4•5 years ago
|
||
Currently, only HTMLInputElement
reuses TextControlState
instance since
HTMLTextAreaElement
had the instance as a member rather than allocate it.
Now, all instances are allocated in the heap independently for guaranteeing
their lifetime. So, the reuse mechanism should be managed by
TextControlState
itself.
Depends on D51393
Assignee | ||
Comment 5•5 years ago
|
||
TextControlState::SetValue()
does 4 things.
- Committing composition if there is and if possible.
- Setting value with
TextEditor
if text editor and frame are available. - Setting value without
TextEditor
otherwise. - Notifying value changed.
We can split #2 and #3 from it now because AutoTextControlHandlingState
manages nested actions. Therefore, this patch creates
SetValueWithTextEditor()
and SetValueWithoutTextEditor()
which take
AutoTextControlHandlingState
.
Depends on D51394
Assignee | ||
Comment 6•5 years ago
|
||
Currently, "input" event is fired when the AutoScriptBlocker
in SetValue()
is deleted. So, for keeping same behavior, the post processing after calling
TextEditor
methods should be done before editor dispatches "input" event.
Fortunately, TextInputListener::OnEditActionHandled()
is a good chance to
do that. Therefore, this patch makes it notify TextControlState
and
AutoTextControlHandlingState
.
Note that ideally, each method of TextEditor
should return
NS_ERROR_OUT_OF_MEMORY
coming from
AutoTextControlHandlingState::OnEditActionHandled()
. However, it requires
a lot of changes in editor classes, and the case is really rare since editor
does not use fallible allocation. Therefore, it must be okay to crash in
editor if OnEditActionHandled()
returns NS_ERROR_OUT_OF_MEMORY
.
Depends on D51395
Assignee | ||
Comment 7•5 years ago
|
||
For allowing TextEditor
to dispatch DOM events synchronously, we should
remove AutoScriptBlocker
in TextControlState::SetValue()
right now.
According to the comment around the AutoScriptBlocker
, PrepareEditor()
may be called while setting value. Therefore, this patch makes
AutoTextControlHandlingState
call it if PrepareEditor()
is called
while handling SetValue()
and when the top most SetValue()
ends its job.
Depends on D51396
Comment 8•5 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #0)
It was introduced by bug 534785:
https://bugzilla.mozilla.org/attachment.cgi?id=447636&action=diffThe comment which was posted with the patch is:
Today I finally managed to reproduce this. The problem was that
IsPreformatted
's call toFlushPendingNotifications
causes thePrepareEditor
event to run, which proceeded to callnsPlaintextEditor::Init
, which caused thensTextEditRules
object on the stack being deleted. The correct fix here is to add a script blocker on the stack innsTextEditorState::SetValue
to make sure that any pendingPrepareEditor
calls are not executed untilInsertText
is fully executed.Fortunately, we don't have
TextEditRules
(norHTMLEditRules
) anymore (they were merged into the editor classes) and this blocksTextEditor
to dispatchbeforeinput
event as cancelable. So, I need to remove the script blocker from it.
Nice! Inconsistencies between the lifetimes of the rules and the editor objects were the source of so many subtle bugs over the years, glad to hear the rules object is gone now!
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/88c5fa54553b
https://hg.mozilla.org/mozilla-central/rev/191c2592d268
https://hg.mozilla.org/mozilla-central/rev/5915226eefd5
https://hg.mozilla.org/mozilla-central/rev/041047456efa
https://hg.mozilla.org/mozilla-central/rev/7754b00fcfc8
https://hg.mozilla.org/mozilla-central/rev/7608c252368e
https://hg.mozilla.org/mozilla-central/rev/6cc550e9edcd
Description
•