Allocator mismatch related to js::wasm

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jseward, Assigned: luke)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

For the test case dom/promise/tests/test_webassembly_compile.html
valgrind reports memory being allocated with malloc() but being freed
with operator delete(void*).

STR:
DISPLAY=:1.0 ./mach mochitest --keep-open=no -f plain \
  --valgrind=/home/sewardj/VgTRUNK/progress/Inst/bin/valgrind \
  --valgrind-args=--num-transtab-sectors=24,--px-default=allregs-at-mem-access,--px-file-backed=unwindregs-at-mem-access,--fair-sched=yes,--fullpath-after=/MOZ/,--trace-children=yes,--show-mismatched-frees=yes \
  dom/promise/tests/test_webassembly_compile.html
Valgrind does sometimes falsely report such errors due to being
confused by inlining, but I think this one is legitimate.

Checking the last few frames of the allocation and deallocation 
paths gives the results below, but in short, the allocation and
deallocation paths really do lead to malloc() and operator delete
respectively.  So the report looks valid.

ALLOCATION:

WasmGenerator.cpp:121: ModuleGenerator::init: 
   metadata_ = js_new<Metadata>();
->
Utility.h:346 js_new<js::wasm::Metadata>:
   T* ptr = NEWNAME<T>(mozilla::Forward<Args>(args)...); with NEWNAME = js_malloc
->
Utility.h:229 js_malloc(size_t bytes):
   return malloc(bytes);

so the allocation chain really does lead to malloc().  The deallocation
chain finishes like this:

DEALLOCATION:

WasmModule.h:135 js::wasm::Module::~Module()
    ~Module() override { /* Note: can be called on any thread */ }
->
RefPtr.h:78 ~RefPtr
    ConstRemovingRefPtrTraits<T>::Release(mRawPtr);
->
RefPtr.h:409 Release
    mozilla::RefPtrTraits<U>::Release(const_cast<U*>(aPtr));
->
RefPtr.h:40 Release
    aPtr->Release();
->
RefCounted.h:135 Release
        delete static_cast<const T*>(this);
The allocation/deallocation steering machinery in js/public/Utility.h
looks complex and I don't begin to understand it.  What I can say is,
I have the impression that this is related to Wasm's use of said
machinery, rather than to the machinery itself.  That's because these
reports always have Wasm* frames in the stacks, and I assume that
Wasm is not the only user of js/public/Utility.h.  I see no mismatches
reported that don't involve Wasm.
Arg; I think the root of the problem here is that we can't use mozilla::RefCounted in SM; we'll need to add our own js::RefCounted in js/public that uses js_delete, as we do with various other mfbt utilities.
Ok, giving this the js::UniquePtr treatment.  Julian, does this fix the warnings?
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8816546 - Flags: review?(jwalden+bmo)
Attachment #8816546 - Flags: feedback?(jseward)
Comment on attachment 8816546 [details] [diff] [review]
fix-allocators

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

::: mfbt/RefCounted.h
@@ +15,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Move.h"
>  #include "mozilla/RefCountType.h"
>  #include "mozilla/TypeTraits.h"
> +#include "mozilla/UniquePtr.h"

I'm mildly uncertain about importing all of UniquePtr.h for this, versus putting DefaultDelete into a separate header that both can include, but eh.  Probably good enough.
Attachment #8816546 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
Yeah, I asked myself the same question and came to the same conclusion.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f38852711f38
Add js::RefCounted that uses js_delete (r=waldo)
backed out for bustage like f38852711f38
Flags: needinfo?(luke)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d08aeb18ac9
Backed out changeset f38852711f38 for bustage on a CLOSED TREE
Mmm, surprise template specialization in ElfLoader.cpp.
Flags: needinfo?(luke)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a52f03ddcb0
Add js::RefCounted that uses js_delete (r=waldo)
https://hg.mozilla.org/mozilla-central/rev/3a52f03ddcb0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Sorry to arrive late back at the party.  This doesn't seem to fix it,
or perhaps it partially fixes it, but not entirely.  For example, see

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c94b0be8e64fea1ef8f887c487af6b5628b69c&exclusion_profile=false

Click on "Excluded Jobs".  You should then be able to see the 40 valgrind-run 
chunks inside "tc-M-V [tier 2]".  The failures are visible in chunks 2, 5 and 14.
Ahh: we fixed the problem for wasm::Module, but we have a 'using mozilla::RefCounted' so our other internal uses of bare 'RefCounted' were picking up mozilla::RefCounted, not js::RefCounted.
Posted patch fix-ref-counted (obsolete) — Splinter Review
Attachment #8821165 - Flags: review?(bbouvier)
Attachment #8821165 - Flags: feedback?(jseward)
Attachment #8816546 - Flags: feedback?(jseward)
Luke, forgot to qref? :)
Flags: needinfo?(luke)
D'oh!
Attachment #8821165 - Attachment is obsolete: true
Attachment #8821165 - Flags: review?(bbouvier)
Attachment #8821165 - Flags: feedback?(jseward)
Flags: needinfo?(luke)
Attachment #8821179 - Flags: review?(bbouvier)
Attachment #8821179 - Flags: feedback?(jseward)
Attachment #8821179 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b106eda10bfa
Baldr: convert other mozilla::RefCounted to js::RefCounted (r=bbouvier)
(In reply to Luke Wagner [:luke] from comment #18)
> Created attachment 8821179 [details] [diff] [review]
> fix-ref-counted

LGTM.
Comment on attachment 8821179 [details] [diff] [review]
fix-ref-counted

Great, thanks for following up!
Attachment #8821179 - Flags: feedback?(jseward)
You need to log in before you can comment on or make changes to this bug.