Bug 116661

Created attachment 202668 [details] Patch

Comment on attachment 202668 [details] Patch Clearing flags on attachment: 202668 Committed r150600: <http://trac.webkit.org/changeset/150600>

All reviewed patches have been landed. Closing bug.

The patch was rolled out in http://trac.webkit.org/changeset/150621 because it broke Windows tests.

(In reply to comment #4) > The patch was rolled out in http://trac.webkit.org/changeset/150621 because it broke Windows tests. r150621 says "as it breaks the VS2010 builds" and nothing about broken tests. a) I built the patch with VS2010 and have no problem and b) I don't see the test failure at the bots

This change caused reproducible run-time crashes on all of our VS2010 build bots. I was mistaken in my comment that it caused a build failure; this was a run-time failure. For some reason, VS2005 builds seem to work properly with this patch. Perhaps something in the compiler changed between releases. Specific test failures were in the JavaScriptCore test suite.

(In reply to comment #6) > This change caused reproducible run-time crashes on all of our VS2010 build bots. I was mistaken in my comment that it caused a build failure; this was a run-time failure. Which VS2010 bot? Where are they??? Since when is the VS2010 build "official"? > For some reason, VS2005 builds seem to work properly with this patch. Perhaps something in the compiler changed between releases. > > Specific test failures were in the JavaScriptCore test suite. Please show me a link the failure or tell me which test makes problem. What is the _exact_ platform. BTW: Please add a comment to the bug when rolling out. You rolled out _two_ patches and did not add any comment. Also it's hard to fix it when a) ChangeLog is wrong and there are no information about what's the exact problem. :-/

CC'ing Mark Lam for JSCore review.

Comment on attachment 202668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202668&action=review > Source/WTF/wtf/StackBounds.cpp:204 > + m_bound = static_cast<char*>(m_origin) - (reservedMemory.RegionSize - guardPage.RegionSize + committedMemory.RegionSize) + pageSize; Are you sure that this formula is correct? I applied this patch in a local build and dumped all the BaseAddreses and RegionSizes. It looks like the 3 regions (uncommitted, guard page, and committed) are mutually exclusive. Note: according to the data I got from applying your patch, the "reservedMemory" region would be more accurately named "reservedButUncommittedMemory". I'll just use "uncommittedMemory" since the guardPage and committed regions are also "reserved". The size of the reserved address range should be (uncommitedMemory.RegionSize + guardPage.RegionSize + commitedMemory.RegionSize). The useable stack address range should therefore be (uncommittedMemory.RegionSize + committedMemory.RegionSize). This is in accordance with the specs at http://msdn.microsoft.com/en-us/library/ms686774%28VS.85%29.aspx which states that "The system commits additional pages from the reserved stack memory as they are needed, until either the stack reaches the reserved size minus one page (which is used as a guard page to prevent stack overflow) …". Hence, m_bound should be (with reservedMemory renamed to uncommittedMemory): m_bound = static_cast<char*>(m_origin) - (uncommittedMemory.RegionSize + committedMemory.RegionSize); Can you please check the address ranges again and confirm if this is the case?

(In reply to comment #7) > BTW: Please add a comment to the bug when rolling out. You rolled out _two_ patches and did not add any comment. Also it's hard to fix it when a) ChangeLog is wrong and there are no information about what's the exact problem. :-/ BTW, regarding the test that failed, I'm seeing failures when running Tools/Scripts/run-javascriptcore-tests. I know of at least 2 issues when running these tests: 1. There's an assertion failure in the StackPolicy in Interpreter.cpp. The StackPolicy expects that the total stack size must be greater than 256k. In my test run on Windows, the total reserved space of the stack region is 256k. Minus the 4k guard page, it's certainly less than 256k. This 256k value in the StackPolicy was picked based on some heuristics. We'll need to rethink how we want to handle this in light of Windows having a stack just under that. Not sure if this is the OS default or something we configured. 2. Once I reduced the StackPolicy requirement in my local Windows build, I saw another assertion failure in the testapi test (which is part of run-javascriptcore-tests). I have yet to isolate the root cause of this assertion failure. Still investigating.

I've filed https://bugs.webkit.org/show_bug.cgi?id=117801 to deal with the assertion failure issues, and blocked this bug on that one. Apart from that, I still think that there is a bug in this patch as indicated in comment #9.

Created attachment 205128 [details] patch to roll Patrick's patch back in

Comment on attachment 205128 [details] patch to roll Patrick's patch back in r=me

The patch has been rolled back in in r151810: <http://trac.webkit.org/changeset/151810>.