Status

()

defect
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: sayrer, Assigned: sayrer)

Tracking

({fixed1.9.1})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

11 years ago
Editor holds references to input elements as soon as you open the browser.
Assignee

Comment 1

11 years ago
Posted patch partial patch (obsolete) — Splinter Review
Assignee

Updated

11 years ago
Attachment #309723 - Attachment is patch: true
Attachment #309723 - Attachment mime type: application/octet-stream → text/plain
Assignee

Comment 2

11 years ago
Comment on attachment 309723 [details] [diff] [review]
partial patch

This patch identifies some references to nsGenericElements that were previously unaccounted for.
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
Posted patch partial patch, updated (obsolete) — Splinter Review
attachment 309723 [details] [diff] [review] basically still applied, except for some whitespace-only changes that we're better off without anyway.  I also changed a drop of commenting-out into removal.

This fixes bug 420670.

Sayre, was there any reason you hadn't requested review on this back when you wrote it?
Attachment #309723 - Attachment is obsolete: true
Assignee

Comment 4

10 years ago
(In reply to comment #3)
> 
> Sayre, was there any reason you hadn't requested review on this back when you
> wrote it?

I think it's because I couldn't actually show improved CC behavior. I assumed it was docshell's alleged thread safety preventing adequate collection. But since you find it fixes bugs, we should probably take it.
So I can't reproduce the testing that I did a few days ago that shows it fixes bug 420670.  But I *can* reproduce that this *plus* bug 488799 (add editor transactions and transaction manager to cycle collector) fixes bug 420670.  And I tried it about 3 times both with and without the patch this time.

That said, the above patch also makes crashtests crash.  I'll post a patch with the fix; nsHTMLEditRules derives from nsTextEditRules but (without my revision) didn't forward to its QueryInterface.  My patch makes the QueryInterface implementation forward (and thus get the cycle collection helper and cycle collection isupports entries) and also switches the canonical nsISupports pointer to the forwarded one.
Attachment #372485 - Attachment is obsolete: true
Attachment #373270 - Flags: superreview?(peterv)
Attachment #373270 - Flags: review?(peterv)
Attachment #372485 - Flags: review?(peterv)
Comment on attachment 373270 [details] [diff] [review]
partial patch, udpated to fix crash

>diff --git a/editor/libeditor/text/nsTextEditRules.cpp b/editor/libeditor/text/nsTextEditRules.cpp

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsTextEditRules)
>+  NS_INTERFACE_MAP_ENTRY(nsIEditRules)

This is missing the entry for nsISupports.

Adding unlinking always makes me nervous in case we end up executing code that didn't expect the pointers to be null. We'll wee how it goes :-).
Attachment #373270 - Flags: superreview?(peterv)
Attachment #373270 - Flags: superreview+
Attachment #373270 - Flags: review?(peterv)
Attachment #373270 - Flags: review+
Posted patch pre-patchSplinter Review
This fixes the crashes I was seeing on mochitests on Mac only (why Mac only?  maybe the ordering of malloc influencing the ordering of Unroot?).

In addition, I added the following two lines to the main patch:

+  if (tmp->mRules)
+    tmp->mRules->DetachEditor();

in nsPlaintextEditor's unlink method.
Attachment #376341 - Flags: superreview?(peterv)
Attachment #376341 - Flags: review?(peterv)
Attachment #376341 - Flags: superreview?(peterv)
Attachment #376341 - Flags: superreview+
Attachment #376341 - Flags: review?(peterv)
Attachment #376341 - Flags: review+
Comment on attachment 373270 [details] [diff] [review]
partial patch, udpated to fix crash

These patches make leak-monitor and DEBUG_CC a good bit less noisy, and may well fix some more permanent leaks.
Attachment #373270 - Flags: approval1.9.1?
Comment on attachment 376341 [details] [diff] [review]
pre-patch

a=shaver
Attachment #376341 - Flags: approval1.9.1? → approval1.9.1+
Looks like this never got resolved.  Marking fixed.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.