If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Protect IsPreFormatted callers from premature death

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

({regression})

Trunk
mozilla1.9.3a5
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Bug 336104 made IsPreFormatted flush pending notifications.  In some cases, that could lead to the object calling IsPreFormatted being destroyed, which can lead to crashes or worse.  We should protect those objects against premature death.
(Assignee)

Comment 1

8 years ago
For an example of this problem, see bug 534785 comment 44.
(Assignee)

Comment 2

8 years ago
Created attachment 447640 [details] [diff] [review]
Patch (v1)

nsWSRunObject::GetRuns is safe, because that object is not refcounted.
Attachment #447640 - Flags: review?(roc)
Can we move these higher in the call stack? It's fragile to let an object die while we're in its methods, so it's better to have death-grips on objects other than 'this'...
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> Can we move these higher in the call stack? It's fragile to let an object die
> while we're in its methods, so it's better to have death-grips on objects other
> than 'this'...

Yes, we can move these to higher in the stack, but we still need to hold the death grips on the edit rules objects, which are those which actually die.  That's why I added the death grips to the direct callers (and to minimize the risk of missing any callers.)
You mean the rules object dies but the editor doesn't?
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> You mean the rules object dies but the editor doesn't?

Yes.  The rules object dies because the Init method on the editor is called again, which deletes the old rules object and creates a new one in place of it, leaving the old object in a very sad state.
Shouldn't we just reuse the existing object in that case? Would that solve these cases you're protecting against?
(Assignee)

Comment 8

8 years ago
Created attachment 447901 [details] [diff] [review]
Patch (v2)

(In reply to comment #7)
> Shouldn't we just reuse the existing object in that case? Would that solve
> these cases you're protecting against?

Yes, it should.  And that sounds like a more robust thing to do anyway, so let's go for that.
Attachment #447640 - Attachment is obsolete: true
Attachment #447901 - Flags: review?(roc)
Attachment #447640 - Flags: review?(roc)
Attachment #447901 - Flags: review?(roc) → review+
(Assignee)

Comment 9

8 years ago
Comment on attachment 447640 [details] [diff] [review]
Patch (v1)

(In reply to comment #7)
> Shouldn't we just reuse the existing object in that case? Would that solve
> these cases you're protecting against?

As it turns out, reusing the object isn't very practical.  This object stores all of the details of the editing state for an editor, and reusing the existing object breaks a lot of assumptions inside the editor code.  I tried to fix a few of them, but there are still a lot more problems remaining, and I don't think that this approach is as good as I thought it is now.

So I think it would be better to go with this version of the patch.
Attachment #447640 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #447901 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #447640 - Attachment is obsolete: false
Can we move the death-grips out of the methods of the edit-rules objects and into methods of the editor?
(Assignee)

Comment 11

8 years ago
Created attachment 448217 [details] [diff] [review]
Patch (v3)

(In reply to comment #10)
> Can we move the death-grips out of the methods of the edit-rules objects and
> into methods of the editor?

Sure.
Attachment #447640 - Attachment is obsolete: true
Attachment #448217 - Flags: review?(roc)
Attachment #447640 - Flags: review?(roc)
Comment on attachment 448217 [details] [diff] [review]
Patch (v3)

Ick. I hope we can make these go away eventually by making editor state survive reframing.
Attachment #448217 - Flags: review?(roc) → review+
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5276a2d5f463
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.