1417504 - Windows ASan malloc_usable_size error in mozilla::dom::Element::AddSizeOfExcludingThis

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(Away)]

(To reproduce this currently requires a bunch of patches to work around other issues, but I believe the error is legitimate.)

==12452==ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x11be368b71a0
    #0 0x7ffa1674c467  (e:\test\asan\firefox\clang_rt.asan_dynamic-x86_64.dll+0x18001c467)
    #1 0x7ffa056d68ee in mozilla::dom::Element::AddSizeOfExcludingThis(class nsWindowSizes &,unsigned __int64 *)const  z:\build\build\src\dom\base\Element.cpp:4440
    #2 0x7ffa05995c01 in AddSizeOfNodeTree z:\build\build\src\dom\base\nsDocument.cpp:12879
    #3 0x7ffa05994966 in nsDocument::DocAddSizeOfExcludingThis(class nsWindowSizes &)const  z:\build\build\src\dom\base\nsDocument.cpp:12924
    #4 0x7ffa0529dcc4 in mozilla::image::VectorImage::SizeOfSourceWithComputedFallback(class mozilla::SizeOfState &)const  z:\build\build\src\image\VectorImage.cpp:395
    #5 0x7ffa052d661d in imgRequest::UpdateCacheEntrySize(void) z:\build\build\src\image\imgRequest.cpp:631
    
Looks like pointer came from HeapAlloc (not jemalloc):

3:036> !address 11be368b71a0
[...]
More info:              heap owning the address: !heap 0x13e23410000

Modulo some inlining, I believe this is: https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/dom/base/Element.cpp#4458

Where `sc`, coming from servo-land, is HeapAlloc-based on Windows.

njn, does this memory reporting need to be adjusted somehow for the dual-allocator situation on Windows?

Comment 1

[(Away)]

Although... I thought we disabled jemalloc on asan builds anyway. Nonetheless, maybe the servo allocator situation is still causing some confusion.

Comment 2

[(Away)]

Oh: in asan builds, maybe Gecko allocations go through C malloc, and Servo ones go directly to HeapAlloc?

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to David Major [:dmajor] from comment #2)
> Oh: in asan builds, maybe Gecko allocations go through C malloc, and Servo
> ones go directly to HeapAlloc?

glandium said on IRC: "it's entirely possible, we only wrap HeapAlloc when MOZ_MEMORY is defined".

He also said: "rust *always* uses HeapAlloc on windows when building without rust-jemalloc".

In which case, yes, this is a clear mismatch. The surprising thing is that it hasn't come up earlier. I would have thought any ASAN builds on Windows would be pretty much guaranteed to hit this any time you ran memory reporting. (Maybe ASAN-on-Windows is still a rare configuration?)

Comment 4

[(Away)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> In which case, yes, this is a clear mismatch. The surprising thing is that
> it hasn't come up earlier. I would have thought any ASAN builds on Windows
> would be pretty much guaranteed to hit this any time you ran memory
> reporting. (Maybe ASAN-on-Windows is still a rare configuration?)

Yes, it hits within seconds of browser startup. :) And only after patching around an even earlier startup crash. Our Windows ASan builds are indeed still quite young.

Comment 5

[Nicholas Nethercote [inactive]]

There are two problems here.

- ASan will not be wrapping HeapAlloc and HeapFree, which means it's missing important information.

- We are allocating blocks in Rust and deallocating them in C++, or vice versa. As evidenced by the fact that normal builds run fine and Windows ASan builds work if Stylo is disabled.

glandium, you said that we need to enable HeapAlloc wrapping on those builds. This will fix both problems.

Comment 6

[(Away)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> - We are allocating blocks in Rust and deallocating them in C++, or vice
> versa. As evidenced by the fact that normal builds run fine and Windows ASan
> builds work if Stylo is disabled.

Do we really have mixed deallocation happening? All I see here is that we're allocating in rust/HeapAlloc and _measuring_ in C++/jemalloc.

Comment 7

[Mike Hommey [:glandium]]

Same problem anyways, rust shouldn't be using HeapAlloc.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1417504 - Also wrap Heap{Alloc,ReAlloc,Free} when building without our allocator.

Bug 1280578 added some wrapping for the Win32 Heap* functions, mainly
for the rust static libraries that use them. Because pointer ownership
might cross the C++/Rust boundary, and because about:memory uses
malloc_usable_size/msize, we need both C++ and Rust to still use the
same heap on builds where our allocator is not enabled.

Review commit: https://reviewboard.mozilla.org/r/208316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208316/

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8937640 [details]
Bug 1417504 - Also wrap Heap{Alloc,ReAlloc,Free} when building without our allocator.

https://reviewboard.mozilla.org/r/208316/#review214072

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f4691a291cf
Also wrap Heap{Alloc,ReAlloc,Free} when building without our allocator. r=njn

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5f4691a291cf

Comment 12

[(Away)]

Thank you glandium! Windows ASan builds are quite usable now. I did some basic browsing without problems.