Closed Bug 1320905 Opened 7 years ago Closed 7 years ago

Allocator mismatch related to js::wasm


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: jseward, Assigned: luke)



(3 files, 1 obsolete file)

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*).

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 \
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.


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:


WasmModule.h:135 js::wasm::Module::~Module()
    ~Module() override { /* Note: can be called on any thread */ }
RefPtr.h:78 ~RefPtr
RefPtr.h:409 Release
RefPtr.h:40 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.
Attached patch fix-allocatorsSplinter Review
Ok, giving this the js::UniquePtr treatment.  Julian, does this fix the warnings?
Assignee: nobody → luke
Attachment #8816546 - Flags: review?(jwalden+bmo)
Attachment #8816546 - Flags: feedback?(jseward)
Comment on attachment 8816546 [details] [diff] [review]

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
Add js::RefCounted that uses js_delete (r=waldo)
backed out for bustage like f38852711f38
Flags: needinfo?(luke)
Backout by
Backed out changeset f38852711f38 for bustage on a CLOSED TREE
Mmm, surprise template specialization in ElfLoader.cpp.
Flags: needinfo?(luke)
Pushed by
Add js::RefCounted that uses js_delete (r=waldo)
Closed: 7 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

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.
Attached 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)
Attached patch fix-ref-countedSplinter Review
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
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

Comment on attachment 8821179 [details] [diff] [review]

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.