Closed
Bug 1341889
Opened 7 years ago
Closed 7 years ago
Add some other checks to determine where realloc is going wrong
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ehoogeveen, Assigned: ehoogeveen)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
7.79 KB,
patch
|
ehoogeveen
:
review+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
ehoogeveen
:
review+
|
Details | Diff | Splinter Review |
The fixes in bug 1339441 didn't help, so let's back up a bit and narrow down where the problem originates a bit more.
Assignee | ||
Comment 1•7 years ago
|
||
This makes pretty liberal use of the new MOZ_CRASH_UNSAFE_PRINTF. I'm skeptical that storing the previous size and address will tell us anything, but it should be interesting to see what malloc_usable_size tells us about what jemalloc thinks the underlying buffer size is. Getting that working everywhere was the trickiest part - there might be a better way to declare malloc_usable_size, but this works. I confirmed locally that MOZ_MEMORY is defined as expected in all normal builds. Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eaad8b9f1fd9cf8608f56f61b0859d35607ea10 I'm confident that this works on Windows and Linux; hopefully OSX will be green as well.
Attachment #8840136 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
Something went wrong with the landing of bug 1338574. This depends on that getting fixed.
Depends on: 1338574
Assignee | ||
Updated•7 years ago
|
Attachment #8840136 -
Attachment description: Add more checks during reallocation to rule out other possibilities. → Part 1: Add more checks during reallocation to rule out other possibilities.
Assignee | ||
Comment 3•7 years ago
|
||
Adding uses of malloc_usable_size made the browser hazard analysis blow up, so add a suppression. I'm not entirely sure why the existing annotations for malloc_table_t and malloc_hook_table_t aren't working, since I'm pretty sure it's the replace_malloc function pointers that are causing the hazards. Also, why is this showing up at all? The pointer we're passing to malloc_usable_size is just the AssemblerBuffer's underlying vector, but that shouldn't itself be a GC thing, right? Try run with that change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=533f5e4c45b6ccebcf39c84b1503304ec39012bf
Attachment #8840232 -
Flags: review?(sphink)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3) > Also, why is this showing up at all? The pointer we're passing to > malloc_usable_size is just the AssemblerBuffer's underlying vector, but that > shouldn't itself be a GC thing, right? I guess it's the other way around - the analysis assumes that malloc_usable_size might do anything, including triggering a GC, and it's worried about other objects that aren't rooted.
Comment 5•7 years ago
|
||
Comment on attachment 8840136 [details] [diff] [review] Part 1: Add more checks during reallocation to rule out other possibilities. Review of attachment 8840136 [details] [diff] [review]: ----------------------------------------------------------------- Please file a P1 or P2 bug in the JIT component to back this out later? We should try this but these defines make me a little uncomfortable :) ::: js/src/ds/PageProtectingVector.h @@ +536,5 @@ > + if (usableSize < currSize) { > + MOZ_CRASH_UNSAFE_PRINTF("maybe_pod_realloc: usableSize < currSize " > + "(%" PRIu64 " < %" PRIu64 ", %" PRIu64 ", %s)!", > + uint64_t(usableSize), uint64_t(currSize), > + uint64_t(prevSize), prevAddr == currAddr ? "true" : "false"); Wouldn't just adding %p, %p for prevAddr, currAddr give us strictly more information? Not sure how useful the addresses are so feel free to ignore.
Attachment #8840136 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > Please file a P1 or P2 bug in the JIT component to back this out later? We > should try this but these defines make me a little uncomfortable :) Will do. This alloc policy is turning into more and more of a hack. > Wouldn't just adding %p, %p for prevAddr, currAddr give us strictly more > information? Not sure how useful the addresses are so feel free to ignore. It would, but I limited the amount of arguments MOZ_CRASH_UNSAFE_PRINTF can take to 4 as a safety measure. Maybe that's too restrictive in the long run, but I felt a bit silly bumping the limit up right after it landed.
Assignee | ||
Comment 7•7 years ago
|
||
Filed bug 1342023. > Not sure how useful the addresses are so feel free to ignore. To add to the above, I don't think the addresses are actually useful by themselves. If currAddr doesn't match oldAddr we'll already crash above, and in that case we *do* print the addresses. prevAddr being corrupted wouldn't tell us much, since it doesn't get set until we're already done with the buffer it pointed to; it's only really interesting to tell us whether the previous realloc was in-place.
Comment 8•7 years ago
|
||
Comment on attachment 8840232 [details] [diff] [review] Part 2: Add a hazard suppression for malloc_usable_size. Review of attachment 8840232 [details] [diff] [review]: ----------------------------------------------------------------- I could look at the hazards.txt file to try to figure out why this suppression was required, but you may as well just land this anyway.
Attachment #8840232 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 10•7 years ago
|
||
Oh right, I needed to update the commit message. Man, I'm out of it today.
Attachment #8840136 -
Attachment is obsolete: true
Attachment #8840632 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8840232 -
Attachment is obsolete: true
Attachment #8840634 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
One more try run to be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=380497f2465a362472c7a759fa5e49ea78bb08ff (feeling paranoid now)
Comment 13•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b836770e5cb1 Part 1: Add more checks during reallocation to rule out other possibilities. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7dddb3428e Part 2: Add a hazard suppression for malloc_usable_size. r=sfink
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b836770e5cb1 https://hg.mozilla.org/mozilla-central/rev/7b7dddb3428e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•