1341889 - Add some other checks to determine where realloc is going wrong

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1: Add more checks during reallocation to rule out other possibilities.

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.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Something went wrong with the landing of bug 1338574. This depends on that getting fixed.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2: Add a hazard suppression for malloc_usable_size.

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

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

(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

[Jan de Mooij [:jandem]]

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.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

(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.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

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

[Steve Fink [:sfink] [:s:]]

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.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks! Let's see what this tells us.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 1: Add more checks during reallocation to rule out other possibilities.

Oh right, I needed to update the commit message. Man, I'm out of it today.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2: Add a hazard suppression for malloc_usable_size.

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

One more try run to be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=380497f2465a362472c7a759fa5e49ea78bb08ff (feeling paranoid now)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b836770e5cb1
https://hg.mozilla.org/mozilla-central/rev/7b7dddb3428e