Closed Bug 1336011 Opened 3 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH | mozilla::EditorBase::DeleteSelectionImpl

Categories

(Core :: Editor, defect, P2, critical)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jchen, Assigned: masayuki)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-f42d7792-8641-46d2-9be9-11ab02170201.
=============================================================

This crash started happening on Windows in the 01-27 Aurora, and has appeared in every Aurora since then, but looking at the regression range for 01-27 [1], I don't see anything that stood out. Any ideas, Masayuki-san?

[1] https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=f0e4b9855df1986fbc3e560b6373bd4f276440e5&tochange=33447b9cb7d008da4f1f5b1a1f1777b712a5c254
Flags: needinfo?(masayuki)
Hmm, looks like that this could be caused by removing some items of mActionListeners during a call of |listener->WillDeleteSelection(selection);|. The regression cause might be outside editor.

Although, I don't know when count of for loop is fixed at runtime, I guess that we should create AutoEditActionListenerFlusher class which notifies editor of preventing to remove from mActionListeners immediately even if RemoveEditAction() is called when a instance is created and notifies editor of flushing to remove the pending items.
Flags: needinfo?(masayuki)
Priority: -- → P2
Okay, I have a simple idea to fix this. Taking.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8834867 [details]
Bug 1336011 part.1 EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop

https://reviewboard.mozilla.org/r/110356/#review112072

do we need these patches in aurora _and_ beta too?
Attachment #8834867 - Flags: review?(bugs) → review+
Comment on attachment 8834868 [details]
Bug 1336011 part.2 Create an alias of the type of mEditorObservers

https://reviewboard.mozilla.org/r/110358/#review112078

::: editor/libeditor/EditorBase.h:1010
(Diff revision 1)
>    // Listens to all low level actions on the doc.
>    typedef AutoTArray<OwningNonNull<nsIEditActionListener>, 5>
>              AutoActionListenerArray;
>    AutoActionListenerArray mActionListeners;
>    // Just notify once per high level change.
> -  nsTArray<OwningNonNull<nsIEditorObserver>> mEditorObservers;
> +  typedef AutoTArray<OwningNonNull<nsIEditorObserver>, 3>

personally I'm not a fan of this kinds of typedefs, since it hides the ownership model when the type actually is used.

But if you prefer this, fine.
Attachment #8834868 - Flags: review?(bugs) → review+
Comment on attachment 8834869 [details]
Bug 1336011 part.3 Create an alias of the type of mDocStateListeners

https://reviewboard.mozilla.org/r/110360/#review112080
Attachment #8834869 - Flags: review?(bugs) → review+
Comment on attachment 8834867 [details]
Bug 1336011 part.1 EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop

https://reviewboard.mozilla.org/r/110356/#review112072

At least, we need to uplift this to Aurora because this crash is reported a lot from Aurora users.
Comment on attachment 8834868 [details]
Bug 1336011 part.2 Create an alias of the type of mEditorObservers

https://reviewboard.mozilla.org/r/110358/#review112078

> personally I'm not a fan of this kinds of typedefs, since it hides the ownership model when the type actually is used.
> 
> But if you prefer this, fine.

I like this better because:
* the original type is too long for 80 characters per line rule
* |auto| is enough useful to store each item
* it's easier to use same type in everywhere
* it's easier to maintain when we need to change the type

Anyway, when everybody needs to check the type, they need to refer the declaration of the member. Then, the type is also defined there. So, I don't think that this makes the code more difficult to read.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a7bf14d56049
part.1 EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop r=smaug
https://hg.mozilla.org/integration/autoland/rev/5576e07a1b21
part.2 Create an alias of the type of mEditorObservers r=smaug
https://hg.mozilla.org/integration/autoland/rev/14d793e7642a
part.3 Create an alias of the type of mDocStateListeners r=smaug
typedefs do make code easier to read, but harder to see the lifetime management, like whether the array has strong pointers to the items.
https://hg.mozilla.org/mozilla-central/rev/a7bf14d56049
https://hg.mozilla.org/mozilla-central/rev/5576e07a1b21
https://hg.mozilla.org/mozilla-central/rev/14d793e7642a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this when you get a chance.
(In reply to Olli Pettay [:smaug] from comment #13)
> typedefs do make code easier to read, but harder to see the lifetime
> management, like whether the array has strong pointers to the items.

I understand, I think that |auto| isn't clear if each retrieved item is grabbed. So, using actual type to receive each item should make it clearer.
Comment on attachment 8834867 [details]
Bug 1336011 part.1 EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop

Approval Request Comment
[Feature/Bug causing the regression]: Not sure.
[User impact if declined]: User may meet the crash at replacing selected text.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No, because this is reported only from Aurora testers.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just getting a copy and use it for |for| loop.
[String changes made/needed]: No.
Attachment #8834867 - Flags: approval-mozilla-aurora?
Comment on attachment 8834868 [details]
Bug 1336011 part.2 Create an alias of the type of mEditorObservers

Approval Request Comment
Follow up patch of part.1. This patch just makes similar member use similar alias for the long type. So, not changing the behavior. Uplifting this helps to merge when somebody needs to fix around here and uplift them.
Attachment #8834868 - Flags: approval-mozilla-aurora?
Comment on attachment 8834869 [details]
Bug 1336011 part.3 Create an alias of the type of mDocStateListeners

Approval Request Comment
Follow up patch of part.1. This patch just makes similar member use similar alias for the long type. So, not changing the behavior. Uplifting this helps to merge when somebody needs to fix around here and uplift them.
Attachment #8834869 - Flags: approval-mozilla-aurora?
Comment on attachment 8834867 [details]
Bug 1336011 part.1 EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop

Fix a crash. Aurora53+.
Attachment #8834867 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8834868 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8834869 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.