8240211: Stack overflow on Windows 32-bit can lead to crash

[arun-joseph]

Issue: The stack pointer is checked close to the stack limit during the last iteration of calling frameLoaded() and then, grows beyond the thread's stack range causing a stack overflow and crashes. This occurs as the stack grows by an amount larger than the reserved zone at the end of the stack.

Fix: Reduce the stack range visible to the thread in StackBounds.cpp similar to Mac and Linux. This causes the stack pointer check to throw a StackOverflowError during the last iteration.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8240211: Stack overflow on Windows 32-bit can lead to crash

Reviewers

• Guru Hb (ghb - Reviewer)
• Kevin Rushforth (kcr - Reviewer)
• Johan Vos (jvos - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/137/head:pull/137
$ git checkout pull/137

[bridgekeeper]

👋 Welcome back ajoseph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

[mlbridge]

Webrevs

• 00: Full (f780c07)

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

I will review this.

@guruhb can you also review it?

[guruhb]

Looks good to me.
I believe you have executed DRT on both 64 & 32 bit build.

[johanvos]

This looks correct and very valuable to me. Would be great if there was a test that results in a crash before?

[kevinrushforth]

Here is a pointer to the apply-style-iframe-crash.html test case that crashes on Windows 32-bit without this fix and passes with this fix.

[kevinrushforth]

I did a full build / test and verified that on Windows 32-bit the apply-style-iframe-crash.html test crashes without the fix and passes with the fix.

[openjdk]

@arun-joseph This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8240211: Stack overflow on Windows 32-bit can lead to crash

Reviewed-by: ghb, kcr, jvos

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 337ed722d54564d381a4536ba6d2a8b124becae9.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@guruhb, @kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[arun-joseph]

/integrate

[openjdk]

@arun-joseph
Your change (at version f780c07) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

@kevinrushforth @arun-joseph The following commits have been pushed to master since your change was applied:

• 337ed72: 8237926: Potential memory leak of model data in javafx.scene.control.ListView

Your commit was automatically rebased without conflicts.

Pushed as commit cf0bba6.

[mlbridge]

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: cf0bba6
Author: Arun Joseph <ajoseph at openjdk.org>
Committer: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-03-05 19:28:25 +0000
URL: https://git.openjdk.java.net/jfx/commit/cf0bba62

8240211: Stack overflow on Windows 32-bit can lead to crash

Reviewed-by: ghb, kcr, jvos

! modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp