Closed Bug 1417504 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: dmajor, Assigned: glandium)

References

Details

Attachments

(1 file)

(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?
Flags: needinfo?(n.nethercote)
Although... I thought we disabled jemalloc on asan builds anyway. Nonetheless, maybe the servo allocator situation is still causing some confusion.
Oh: in asan builds, maybe Gecko allocations go through C malloc, and Servo ones go directly to HeapAlloc?
(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?)
Flags: needinfo?(n.nethercote)
(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.
Blocks: winasan
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.
Flags: needinfo?(mh+mozilla)
(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.
Same problem anyways, rust shouldn't be using HeapAlloc.
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
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
Attachment #8937640 - Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5f4691a291cf
Also wrap Heap{Alloc,ReAlloc,Free} when building without our allocator. r=njn
https://hg.mozilla.org/mozilla-central/rev/5f4691a291cf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Thank you glandium! Windows ASan builds are quite usable now. I did some basic browsing without problems.
You need to log in before you can comment on or make changes to this bug.