The default bug view has changed. See this FAQ.

loader.js is leaked on addon disabling

RESOLVED FIXED in 1.11

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
1.11
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
On current master, loader.js is leaked when we are disabling the addon.
It appeared to me when I've done a memory debugging session on nightly,
which now displays different compartements for system principals.
So that we can see JSM which are leaked!
(Assignee)

Comment 1

5 years ago
Created attachment 633170 [details]
Pull request 465

Some more details, we are mostly leaking loader.js JSM, but not the laoder instance itself.
Here is a patch to fix this.

Updated

5 years ago
Priority: -- → P2
Attachment #633170 - Flags: review-
I was assuming that modules loaded by other modules are unloaded if the loader is unloaded. With that assumption following should have caused unload of loader.

https://github.com/mozilla/addon-sdk/blob/master/python-lib/cuddlefish/app-extension/bootstrap.js#L195

It looks like that's not the case and we should unload such modules manually. In that case I think we should do it either from bootstrap on `shutdown` or immediately after import. Unloading it on first loader unload introduces inconsistent behavior if more than one loader is created.
(Assignee)

Comment 3

5 years ago
Created attachment 637194 [details]
Pull request 473

I had to create a new pull request ...
Assignee: nobody → poirot.alex
Attachment #633170 - Attachment is obsolete: true
Attachment #637194 - Flags: review?(rFobic)
Comment on attachment 637194 [details]
Pull request 473

Please see notes in github before landing.
Attachment #637194 - Flags: review?(rFobic) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 654652 [details]
Pull request 533
Attachment #637194 - Attachment is obsolete: true
Attachment #654652 - Flags: review?(rFobic)

Updated

5 years ago
Duplicate of this bug: 729423
Whiteboard: [MemShrink:P3]
Blocks: 668871
Blocks: 700547
Comment on attachment 654652 [details]
Pull request 533

Just one more thing.
Attachment #654652 - Flags: review?(rFobic) → review+

Comment 8

5 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/149a2d147a5b306a6f15d70ffc061081c8d3b0bd
Bug 764840: Fix loader leak by using Sandboxes and nuking them instead of JSM.

https://github.com/mozilla/addon-sdk/commit/274ed5582b503e27b26afbacfa661bc176b65fc1
Merge pull request #533 from ochameau/bug/764840-use-sandboxes-for-loader

Bug 764840: Fix loader leak by using Sandboxes and nuking them instead of JSM. r=@gozala
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.11
(Assignee)

Comment 9

5 years ago
Oops. Had to back it out as it conflicted with recent ChromeWorker patch.

https://github.com/mozilla/addon-sdk/commit/7bafac0ef200a7a6647840cede7b823b1cedcf3b

`ChromeWorker` isn't injexted in sandboxes, so that loader can't expose it anymore...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

5 years ago
Created attachment 657857 [details]
Pull request 548
Attachment #654652 - Attachment is obsolete: true
Attachment #657857 - Flags: review?(rFobic)
(Assignee)

Updated

5 years ago
Target Milestone: 1.11 → ---
Comment on attachment 657857 [details]
Pull request 548

Please make sure to address comments in pull request before landing.
Attachment #657857 - Flags: review?(rFobic) → review+

Comment 12

5 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d31175e94cc43804ded0e6ecacaac356a1779160
Bug 764840 - Fix loader leak by using Sandboxes and nuking them instead of JSM.

https://github.com/mozilla/addon-sdk/commit/02bf831b06f331a301661126f619cc2e5aeddb18
Merge pull request #548 from ochameau/bug/764840

Bug 764840 - Fix loader leak by using Sandboxes and nuking them instead of JSM. r=@gozala
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.11
You need to log in before you can comment on or make changes to this bug.