1471310 - Clear the Installed flag when removing the MemoryProtectionExceptionHandler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[(Away)]

Attachment: VEHorange

I have a feeling that this may be the reason behind some occasional oranges that I see around `MOZ_ASSERT(!sHandlingException)`.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

What oranges are you seeing using this code path?

I added more uses of the Memory Protection handler with Bug 1437600. I am currently interested in any bug which might be related. I write down the analysis of these crashes in Bug 1263794.

Comment 2

[(Away)]

I don't have access to those bugs.

Most recently I did a try run of switching Windows builds to clang-cl and hit this pretty frequently in xpcshell: https://treeherder.mozilla.org/#/jobs?repo=try&revision=278fc5b212a95fa15123c8887bb4662d0d6242de

But I've seen this assertion on and off for ages on try, I don't recall specific bugs.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8987906 [details] [diff] [review]
VEHorange

Review of attachment 8987906 [details] [diff] [review]:
-----------------------------------------------------------------

Hah, yes indeed.

Comment 4

[(Away)]

My patch merely moved the failure slightly later :(

Assertion failure: tree.empty(), at z:/build/build/src/js/src/ds/MemoryProtectionExceptionHandler.cpp:81

Comment 5

[(Away)]

And worse, I don't even get .dmp files for these failures now.

nbp, I think I'll need your help investigating this...

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

(In reply to David Major [:dmajor] from comment #2)
> I don't have access to those bugs.

Done.

(In reply to David Major [:dmajor] from comment #4)
> Assertion failure: tree.empty(), at
> z:/build/build/src/js/src/ds/MemoryProtectionExceptionHandler.cpp:81

This assertion likely means that we are leaking a JSRuntime when we exit the program, or that we failed to unregister a memory region from the memory protection system (which would report false positive).

I am tempted to just remove this assertion and hope that it would be reported in some other ways by AWSY / DMD.

> Most recently I did a try run of switching Windows builds to clang-cl and
> hit this pretty frequently in xpcshell:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=278fc5b212a95fa15123c8887bb4662d0d6242de
> 
> But I've seen this assertion on and off for ages on try, I don't recall
> specific bugs.

It sounds to me that we the current SEGV handler is removed as soon as we hit a SEGV. But being able to reach the uninstall function would imply that the SEGV got caught and we might have executed the rest of the program without the memory protection SEGV handler.

I guess the easiest fix would be to use the same code in the SEGV handler and in the uninstall function. (i.e. if (sHandlingException.compareExachange(false, true)) … )

Comment 7

[(Away)]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #6)
> I guess the easiest fix would be to use the same code in the SEGV handler
> and in the uninstall function. (i.e. if
> (sHandlingException.compareExachange(false, true)) … )

I'm not sure what this really buys over just clearing sExceptionHandlerInstalled.

> I am tempted to just remove this assertion and hope that it would be
> reported in some other ways by AWSY / DMD.

For very selfish reasons I'd be happy to do this and get unblocked, though I feel somewhat uneasy about it.  Emanuel, any thoughts on this?

Comment 8

[(Away)]

Attachment: Bug 1471310: Disable the emptiness assertion in ProtectedRegionTree.

Emanuel hasn't been on Bugzilla in a while, and while I'd really like his input I can't afford to wait on this bug. We can tweak this later if necessary but for now let's try to get this landed.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

I don't entirely understand whether this is causing oranges or if it's preventing useful output on existing oranges, but either way it doesn't seem like it's catching any actual problems so I'm fine with removing it.

> Emanuel hasn't been on Bugzilla in a while, and while I'd really like his input I can't afford to wait on this bug.

Sorry, I'll be properly back in about a week.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96b22e24de05
Clear the Installed flag when removing the MemoryProtectionExceptionHandler. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a705caf1f2
Disable the emptiness assertion in ProtectedRegionTree. r=nbp

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/96b22e24de05
https://hg.mozilla.org/mozilla-central/rev/83a705caf1f2