1320905 - Allocator mismatch related to js::wasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Julian Seward [:jseward]]

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

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint (one of several)

Comment 2

[Julian Seward [:jseward]]

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);

Comment 3

[Julian Seward [:jseward]]

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.

Comment 4

[Luke Wagner [:luke]]

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.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-allocators

Ok, giving this the js::UniquePtr treatment.  Julian, does this fix the warnings?

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f38852711f38
Add js::RefCounted that uses js_delete (r=waldo)

Comment 9

[Carsten Book [:Tomcat]]

backed out for bustage like f38852711f38

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d08aeb18ac9
Backed out changeset f38852711f38 for bustage on a CLOSED TREE

Comment 11

[Luke Wagner [:luke]]

Mmm, surprise template specialization in ElfLoader.cpp.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a52f03ddcb0
Add js::RefCounted that uses js_delete (r=waldo)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a52f03ddcb0

Comment 14

[Julian Seward [:jseward]]

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.

Comment 15

[Luke Wagner [:luke]]

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.

Comment 16

[Luke Wagner [:luke]]

Attachment: fix-ref-counted

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Luke, forgot to qref? :)

Comment 18

[Luke Wagner [:luke]]

Attachment: fix-ref-counted

D'oh!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b106eda10bfa
Baldr: convert other mozilla::RefCounted to js::RefCounted (r=bbouvier)

Comment 20

[Julian Seward [:jseward]]

(In reply to Luke Wagner [:luke] from comment #18)
> Created attachment 8821179 [details] [diff] [review]
> fix-ref-counted

LGTM.

Comment 21

[Luke Wagner [:luke]]

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

Great, thanks for following up!

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b106eda10bfa