Status

()

Core
Editor
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

({fixed1.9.1})

Trunk
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 309723 [details] [diff] [review]
partial patch
(Assignee)

Updated

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

Comment 2

10 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
Blocks: 420670
Created attachment 372485 [details] [diff] [review]
partial patch, updated

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

9 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.
Attachment #372485 - Flags: review?(peterv)
Depends on: 488799
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.
Created attachment 373270 [details] [diff] [review]
partial patch, udpated to fix crash
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+
Created attachment 376341 [details] [diff] [review]
pre-patch

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?
Attachment #376341 - Flags: approval1.9.1?
Comment on attachment 376341 [details] [diff] [review]
pre-patch

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