Closed Bug 1341889 Opened 3 years ago Closed 3 years ago

Add some other checks to determine where realloc is going wrong

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

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)

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.
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)
Something went wrong with the landing of bug 1338574. This depends on that getting fixed.
Depends on: 1338574
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.
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)
(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 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+
(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.
Blocks: 1342023
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 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+
Thanks! Let's see what this tells us.
Keywords: checkin-needed
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+
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
https://hg.mozilla.org/mozilla-central/rev/b836770e5cb1
https://hg.mozilla.org/mozilla-central/rev/7b7dddb3428e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.