Closed Bug 1313351 Opened 3 years ago Closed 3 years ago

Intermittent leakcheck | default process: 8 bytes leaked (WasmModule)

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: luke)

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(1 file)

Keywords: mlk
Ok, so reading AtomicRefCounted.h, it only does the leak-checking if MOZILLA_INTERNAL_API is #defined and this does not seem to be the case in WasmJS.cpp (where Release() is called which is likely being inlined) so that is definitely a problem.  I don't think this is as simple as #defining MOZILLA_INTERNAL_API within SM either since that #includes nsXPCOM.h.  So I'm thinking we need to turn js::RefCounted into a stripped clone of mozilla::RefCounted instead of doing the 'using'.
Attached patch fix-ref-countedSplinter Review
This patch just clones mozilla::(Atomic)RefCounted into js/public/RefCounted.h, stripping out the leak-checking goo.  It's important that the clone implement the same interface so that it works for mozilla::RefPtr which we reuse as well.  With the leak-checking goo gone, very little code remains so, for readability, I untemplated the impl.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8821590 - Flags: review?(jdemooij)
Comment on attachment 8821590 [details] [diff] [review]
fix-ref-counted

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

Thanks!

::: js/public/RefCounted.h
@@ +29,5 @@
>  
> +  protected:
> +    RefCounted() : mRefCnt(0) {}
> +    ~RefCounted() { MOZ_ASSERT(mRefCnt == DEAD); }
> +  

Nit: trailing whitespace here and a few times below.
Attachment #8821590 - Flags: review?(jdemooij) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d317cfa267
Fix js::RefCounted to not do leak checking. r=jandem
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39a2a96f6cda
followup - Also back out the ElfLoader changes from bug 1320905 on a CLOSED TREE. r=red
https://hg.mozilla.org/mozilla-central/rev/39a2a96f6cda
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.