Closed Bug 708260 Opened 8 years ago Closed 8 years ago

Make the spell checking UI not hold on to a document in memory when spelling suggestion is invoked through the context menu.

Categories

(SeaMonkey :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 3 obsolete files)

See Firefox Bug 708071 (Spelling suggestions creates a zombie compartment):

Steps to reproduce:

1. In a text area, type a misspelled word.
2. Right-click on the misspelled word so that spelling suggestions appear in the context menu.
3. Close the tab. Open about:memory?verbose and minimize memory usage a dozen times.

A zombie compartment is created and it stays until I repeat it in another text area from another domain. But then the new compartment becomes a zombie. It is created whether I select a suggestion or not.
If I do not right-click on a misspelled word, or if I right-click on a correct word, no zombie compartment is created.
Whiteboard: [MemShrink]
Comment 0 is a copy of bug 708071 comment 0, which makes this bug really confusing.

jst said his understanding was that bug 708071 had fixed the zombie compartment, but there was a C++ leak remaining in the spell checker.

Philip, can you clarify what's going on?  Thanks.
> Comment 0 is a copy of bug 708071 comment 0, which makes this bug really confusing.
Sorry. This is a port bug to make sure we don't diverge too much from Firefox and Thunderbird.

Some of the changes in bug 708071 are not in shared code (/browser/) so needs to be ported to SeaMonkey. I have no idea how dependent the front end code is on changes to /toolkit/content/InlineSpellChecker.jsm but in the past whenever a firefox bug changes something in the back end as well as the front we usually need to change our browser code to match as well.
Severity: normal → minor
Oh, I missed that the product is SeaMonkey.  I'll remove the MemShrink tag.  Thanks.
Whiteboard: [MemShrink]
Attached patch Patch v1.0 Kill Zombie. (obsolete) — Splinter Review
STR from Bug 708071 Comment 0:

> 1. In a text area, type a misspelled word.
> 2. Right-click on the misspelled word so that spelling suggestions appear in
> the context menu.
> 3. Close the tab. Open about:memory?verbose and minimize memory usage a dozen
> times.
> 
> A zombie compartment is created and it stays until I repeat it in another
> text area from another domain. But then the new compartment becomes a zombie.
> It is created whether I select a suggestion or not.
> If I do not right-click on a misspelled word, or if I right-click on a
> correct word, no zombie compartment is created.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #612236 - Flags: review?(iann_bugzilla)
Comment on attachment 612236 [details] [diff] [review]
Patch v1.0 Kill Zombie.

r- for the moment as mailnews will need fixing too.
http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#97 is one of the locations I think.
As far as I can tell Composer doesn't need fixing.
Attachment #612236 - Flags: review?(iann_bugzilla) → review-
> r- for the moment as mailnews will need fixing too.
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#97
> is one of the locations I think.
What exactly needs fixing here? I'm not following you? I don't think there are is any spell check code here is there?
(In reply to Philip Chee from comment #6)
> > r- for the moment as mailnews will need fixing too.
> > http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#97
> > is one of the locations I think.
> What exactly needs fixing here? I'm not following you? I don't think there
> are is any spell check code here is there?

http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.xul overrides the onpopupshowing/onpopuphiding code from nsContextMenu but still calls gContextMenu.initItems so the onpopuphiding code will still need to call your new hiding code.
Attached patch Patch v1.1 Kill More Zombies (obsolete) — Splinter Review
> r- for the moment as mailnews will need fixing too.
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#97
> is one of the locations I think.

Changes since the last patch:
* Fix mailContextMenus.js => MailContextOnPopupHiding()
* Replace document.popupNode with <menupopup>.triggerNode in related code.
* Copy/Sync related comments from Thunderbird (comment only change).
Attachment #612236 - Attachment is obsolete: true
Attachment #613688 - Flags: review?(iann_bugzilla)
Comment on attachment 613688 [details] [diff] [review]
Patch v1.1 Kill More Zombies

>+/**
>+ * Function to clear out the global nsContextMenu, and in the case when we
>+ * are a threadpane context menu, restore the selection so that a right-click
>+ * on a non-selected row doesn't move the selection.
>+ * @param event the onpopuphiding event
This part of the comment is not correct for SM.
>+ */
>+function MailContextOnPopupHiding(aTarget)

>+/**
>+ * Determines whether the context menu was triggered by a node that's a child
>+ * of the threadpane by looking for an ancestor node with id="threadTree".
>+ * @return true if the popupNode is a child of the threadpane, otherwise false
You could add what @param is in this case too.
>+ */
>+function InThreadPane(aTarget)

Worth a comment here too whilst we're at it?
> function FillMailContextMenu(aTarget)

r=me with those answered/addressed
Attachment #613688 - Flags: review?(iann_bugzilla) → review+
> This part of the comment is not correct for SM.
So what would be correct?

> Worth a comment here too whilst we're at it?
>> function FillMailContextMenu(aTarget)
So what should I write?
Attached patch Patch v1.2 Fix Comments r=IanN. (obsolete) — Splinter Review
>>+ * @param event the onpopuphiding event
> This part of the comment is not correct for SM.
>>+ */
>>+function MailContextOnPopupHiding(aTarget)
Fixed.

>>+ * @return true if the popupNode is a child of the threadpane, otherwise false
> You could add what @param is in this case too.
>>+ */
>>+function InThreadPane(aTarget)
Fixed.

> Worth a comment here too whilst we're at it?
>> function FillMailContextMenu(aTarget)

> * Function to set up the global nsContextMenu, and the mailnews overlay.
> * @param aTarget the target of the popup event
Fixed.

> r=me with those answered/addressed
Requesting moa from Mnyromyr for the mailnews changes.
Attachment #613688 - Attachment is obsolete: true
Attachment #613946 - Flags: superreview?(mnyromyr)
Attachment #613946 - Flags: review+
>>+ * @param event the onpopuphiding event
> This part of the comment is not correct for SM.
>>+ */
>>+function MailContextOnPopupHiding(aTarget)
Fixed.

>>+ * @return true if the popupNode is a child of the threadpane, otherwise false
> You could add what @param is in this case too.
>>+ */
>>+function InThreadPane(aTarget)
Fixed.

> Worth a comment here too whilst we're at it?
>> function FillMailContextMenu(aTarget)

> * Function to set up the global nsContextMenu, and the mailnews overlay.
> * @param aTarget the target of the popup event
Fixed.

> r=me with those answered/addressed
Requesting moa from Mnyromyr for the mailnews changes.
Attachment #613946 - Attachment is obsolete: true
Attachment #613946 - Flags: superreview?(mnyromyr)
Attachment #613947 - Flags: superreview?(mnyromyr)
Attachment #613947 - Flags: review+
Attachment #613947 - Flags: superreview?(mnyromyr) → superreview+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/8efa0fc77320
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.