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

findbar.xml destroy() throws and leaks window if .finder is defined, but .finder.destroy is not a function

ASSIGNED
Assigned to

Status

()

Toolkit
Find Toolbar
ASSIGNED
a year ago
7 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
STR:

1) Open fresh browser
2) Install adblockplus
3) Click the abp toolbar button and select "filter preferences"
4) Close filter dialog window
5) Open about:memory, minimize, measure
6) Note that there is a leaked filters.xul window under the ABP addon

This is window is leaking because findbar.xml is not completing its cleanup.  Before it removes its observers it tries to call .finder.destroy().  In ABP, however, finder.destroy() is not a function and it throws.
(Assignee)

Comment 1

a year ago
Created attachment 8771601 [details] [diff] [review]
Bug 1287237 P1 Make findbar.xul not call .finder.destroy() if its not a function.  r=jaws

Make findbar.xml more cautious in its destroy() function.  As described in comment 0 its currently leaking windows in AdBlockPlus due to throwing here.

I will add another patch with a test before landing.
Attachment #8771601 - Flags: review?(jaws)
Comment on attachment 8771601 [details] [diff] [review]
Bug 1287237 P1 Make findbar.xul not call .finder.destroy() if its not a function.  r=jaws

Review of attachment 8771601 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but please request review on the test from me as well before you check this in.
Attachment #8771601 - Flags: review?(jaws) → review+
Comment on attachment 8771601 [details] [diff] [review]
Bug 1287237 P1 Make findbar.xul not call .finder.destroy() if its not a function.  r=jaws

Review of attachment 8771601 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for filing this and providing a fix ;) It also seems like ABP would be able to fix this issue, but okay.

::: toolkit/content/widgets/findbar.xml
@@ +427,5 @@
>              return;
>            this._destroyed = true;
>  
> +          if (this.browser.finder &&
> +              (typeof this.browser.finder.destroy === "function")) {

nit: you can remove the parentheses and s/===/==/ as you're comparing a string where coercion is not necessary.
(Assignee)

Comment 4

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Thanks for filing this and providing a fix ;) It also seems like ABP would
> be able to fix this issue, but okay.

Yes.  ABP sets a browser.finder without a destroy() method.  That could be fixed, but it seems better to be resilient if we can.

> ::: toolkit/content/widgets/findbar.xml
> @@ +427,5 @@
> nit: you can remove the parentheses and s/===/==/ as you're comparing a
> string where coercion is not necessary.

This confuses me.  Its my understanding === prevents coercion.  Isn't it best practice in JS to use === unless you need coercion?
(In reply to Ben Kelly [:bkelly] from comment #4)
> > ::: toolkit/content/widgets/findbar.xml
> > @@ +427,5 @@
> > nit: you can remove the parentheses and s/===/==/ as you're comparing a
> > string where coercion is not necessary.
> 
> This confuses me.  Its my understanding === prevents coercion.  Isn't it
> best practice in JS to use === unless you need coercion?

There is debate in the Firefox team about this. Where we know coercion is not going to take place, we often use ==, and in many other places coercion actually is the intended behavior. I r+'d the patch because I'm indifferent, and in this case it doesn't make a difference.
Whiteboard: [MemShrink] → [MemShrink:P2]
Are you planning on landing this patch any time soon, Ben?
Flags: needinfo?(bkelly)
(Assignee)

Comment 7

7 months ago
I never found time to write a test.  Should I just land it?
Flags: needinfo?(bkelly)
yeah, with the comments addressed, if you will.
You need to log in before you can comment on or make changes to this bug.