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)
Toolkit
Find Toolbar
Tracking
()
NEW
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.
Reporter | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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 3•8 years ago
|
||
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.
Reporter | ||
Comment 4•8 years 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?
Comment 5•8 years ago
|
||
(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.
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 6•7 years ago
|
||
Are you planning on landing this patch any time soon, Ben?
Flags: needinfo?(bkelly)
Reporter | ||
Comment 7•7 years ago
|
||
I never found time to write a test. Should I just land it?
Flags: needinfo?(bkelly)
Comment 8•7 years ago
|
||
yeah, with the comments addressed, if you will.
Reporter | ||
Comment 9•6 years ago
|
||
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
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•