Allocator mismatch related to js::wasm

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: luke)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
Created attachment 8815230 [details]
Valgrind complaint (one of several)
(Reporter)

Comment 2

2 years ago
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);
(Reporter)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8816546 [details] [diff] [review]
fix-allocators

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+
(Assignee)

Comment 7

2 years ago
(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

2 years ago
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)

Comment 10

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d08aeb18ac9
Backed out changeset f38852711f38 for bustage on a CLOSED TREE
(Assignee)

Comment 11

2 years ago
Mmm, surprise template specialization in ElfLoader.cpp.
Flags: needinfo?(luke)

Comment 12

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a52f03ddcb0
Add js::RefCounted that uses js_delete (r=waldo)

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a52f03ddcb0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
Created attachment 8821165 [details] [diff] [review]
fix-ref-counted
Attachment #8821165 - Flags: review?(bbouvier)
Attachment #8821165 - Flags: feedback?(jseward)
(Assignee)

Updated

2 years ago
Attachment #8816546 - Flags: feedback?(jseward)
Luke, forgot to qref? :)
Flags: needinfo?(luke)
(Assignee)

Comment 18

2 years ago
Created attachment 8821179 [details] [diff] [review]
fix-ref-counted

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+

Comment 19

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b106eda10bfa
Baldr: convert other mozilla::RefCounted to js::RefCounted (r=bbouvier)
(Reporter)

Comment 20

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

LGTM.
(Assignee)

Comment 21

2 years ago
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.