Open Bug 1287237 Opened 8 years ago Updated 2 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect

Tracking

()

People

(Reporter: bkelly, Unassigned)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

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.
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.
(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)
I never found time to write a test.  Should I just land it?
Flags: needinfo?(bkelly)
yeah, with the comments addressed, if you will.
Sorry, I lost track of this.  If some one wants to pick it up and land it, please feel free.
Assignee: ben → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: