Closed Bug 1588745 Opened 9 months ago Closed 8 months ago

Needs to remove nsAutoScriptBlocker from nsTextEditorState::SetValue()

Categories

(Core :: DOM: Editor, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

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 to FlushPendingNotifications causes the PrepareEditor event to run, which proceeded to call nsPlaintextEditor::Init, which caused the nsTextEditRules object on the stack being deleted. The correct fix here is to add a script blocker on the stack in nsTextEditorState::SetValue to make sure that any pending PrepareEditor calls are not executed until InsertText 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.

Priority: -- → P2

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

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

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

TextControlState::SetValue() does 4 things.

  1. Committing composition if there is and if possible.
  2. Setting value with TextEditor if text editor and frame are available.
  3. Setting value without TextEditor otherwise.
  4. 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

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

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

(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=diff

The comment which was posted with the patch is:

Today I finally managed to reproduce this. The problem was that IsPreformatted's call to FlushPendingNotifications causes the PrepareEditor event to run, which proceeded to call nsPlaintextEditor::Init, which caused the nsTextEditRules object on the stack being deleted. The correct fix here is to add a script blocker on the stack in nsTextEditorState::SetValue to make sure that any pending PrepareEditor calls are not executed until InsertText 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.

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!

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/88c5fa54553b
part 1: Rename `nsTextEditorState` to `mozilla::TextControlState` r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/191c2592d268
part 2: Move some `TextControlState` members to stack only class r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/5915226eefd5
part 3: Make `TextControlState` not deleted actually while it handles something r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/041047456efa
part 4: Make `TextControlState` reuse its instance by itself r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/7754b00fcfc8
part 5: Split `TextControlState::SetValue()` r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/7608c252368e
part 6: Post processing of setting value with TextEditor should be done before dispatching "input" event r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/6cc550e9edcd
part 7: Remove `AutoScriptBlocker` from `TextControlState::SetValue()` r=Ehsan
You need to log in before you can comment on or make changes to this bug.