Closed Bug 1445955 Opened 4 years ago Closed 4 years ago

Disable MemoryProtectionExceptionHandler function if any JSRuntime outlive the ProtectedRegionTree

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

In case of JSRuntime leaks (Bug 1445619), we might assert that we still have some registered content as of the pages of the JSRuntime might still be protected.

This bug is about weakening this assertion to only ensure that the tree of protected pages is empty iff we have no leaked JSRuntime.
This patch adds an Atomic which is used to ensure if the tree of memory ranges
can be mutated. It is set/reset by the constructor (assuming we would only have
this static instance).

The Atomic is kept out-side the ProtectionPages class, as it indicates if the
content within the class is dirty or clean. (moving it inside should be fine in
practice, but semantically wrong)
Attachment #8959142 - Flags: review?(emanuel.hoogeveen)
Priority: -- → P1
Comment on attachment 8959142 [details] [diff] [review]
Disable memory protection in case of leaked JSRuntime.

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

Annoying, but worth it to get this working with LifoAlloc.

::: js/src/ds/MemoryProtectionExceptionHandler.cpp
@@ +75,4 @@
>  
> +    ~ProtectedRegionTree() {
> +        // See Bug 1445619: Currently many users of the JS engine are leaking
> +        // the world, unfortunately LifoAllpoc owned by JSRuntimes have

Typo: s/LifoAllpoc/LifoAlloc/

@@ +194,5 @@
>              uintptr_t address = uintptr_t(ExceptionRecord->ExceptionInformation[1]);
>  
>              // If the faulting address is in one of our protected regions, we
>              // want to annotate the crash to make it stand out from the crowd.
> +            if (sProtectedRegionsInit && sProtectedRegions.isProtected(address)) {

Hmm, this doesn't seem strictly necessary but I guess it's better to avoid weird edge cases.
Attachment #8959142 - Flags: review?(emanuel.hoogeveen) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff6ecf81af7
Disable memory protection in case of leaked JSRuntime. r=ehoogeveen
https://hg.mozilla.org/mozilla-central/rev/0ff6ecf81af7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.