582910 - 64-bit win7: TEST-UNEXPECTED-FAIL | e4x/Regress/regress-347155.js | Exited with code -1073741571 during test run

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

link.exe sets default stack size is 1MB. (http://msdn.microsoft.com/en-us/library/8cxs58a6.aspx)

But http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcjsruntime.cpp#1227

1227     JS_SetNativeStackQuota(cx, 128 * sizeof(size_t) * 1024);

stack limit of js engine sets 128 * 8 * 1024 = 1MB (32bit version is 512KB)

So e4x/Regress/regress-347155.js causes stack overflow.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you also expect this to fix this crashtest failure, or should a separate bug be filed on it:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280581006.1280581431.31618.gz
Rev3 WINNT 6.1 x64 mozilla-central opt test crashtest on 2010/07/31 05:56:46
TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/mozilla-central-w764-opt-u-crashtest/build/reftest/tests/content/events/crashtests/recursive-DOMNodeInserted.html | Exited with code -1073741571 during test run

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280502954.1280503374.5513.gz
Rev3 WINNT 6.1 x64 mozilla-central opt test crashtest on 2010/07/30 08:15:54
TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/mozilla-central-w764-opt-u-crashtest/build/reftest/tests/content/events/crashtests/recursive-DOMNodeInserted.html | Exited with code -1073741571 during test run

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, I'm reasonably confident it will also fix it.  And probably also the editor test crash in mochitest-plain-3/5, which also looks like it could recurse deeply.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Was this a regression from http://hg.mozilla.org/mozilla-central/rev/b9f320e24e04 , or has it been a problem since we started running the Win64 tests?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to comment #4)
> Was this a regression from
> http://hg.mozilla.org/mozilla-central/rev/b9f320e24e04 , or has it been a
> problem since we started running the Win64 tests?

I believe that this is from changing compiler to VC++9. But, also, this problem has on VC++10 since stack size is unchanged.

(In reply to comment #2)
> Do you also expect this to fix this crashtest failure, or should a separate bug
> be filed on it:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280581006.1280581431.31618.gz
> Rev3 WINNT 6.1 x64 mozilla-central opt test crashtest on 2010/07/31 05:56:46
> TEST-UNEXPECTED-FAIL |
> file:///C:/talos-slave/mozilla-central-w764-opt-u-crashtest/build/reftest/tests/content/events/crashtests/recursive-DOMNodeInserted.html
> | Exited with code -1073741571 during test run
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280502954.1280503374.5513.gz
> Rev3 WINNT 6.1 x64 mozilla-central opt test crashtest on 2010/07/30 08:15:54
> TEST-UNEXPECTED-FAIL |
> file:///C:/talos-slave/mozilla-central-w764-opt-u-crashtest/build/reftest/tests/content/events/crashtests/recursive-DOMNodeInserted.html
> | Exited with code -1073741571 during test run

I think that this is same.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 461446 [details] [diff] [review]
fix

Someone more familiar with the particularities of x64 and this problem should review this. Maybe ted?

Comment 7

[Justin Dolske [:Dolske]]

(Also, do Linux or OS X need a stacksize adjustment too on x86?)

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 461446 [details] [diff] [review]
fix

Sorry, but this really isn't my purview either. dbaron: care to review?

As an aside, I don't think you need the "see bug 582910" comment. We have blame for people to find bug numbers.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 461446 [details] [diff] [review]
fix

r=dbaron

Though perhaps it's better to use "/STACK" rather tan "-STACK" to be consistent with the code right above it?

(I even found the documentation for the option at
http://msdn.microsoft.com/en-us/library/8cxs58a6%28v=VS.90%29.aspx .)

Comment 10

[Makoto Kato [:m_kato]]

(In reply to comment #9)
> Comment on attachment 461446 [details] [diff] [review]
> fix
> 
> r=dbaron
> 
> Though perhaps it's better to use "/STACK" rather tan "-STACK" to be consistent
> with the code right above it?
> 
> (I even found the documentation for the option at
> http://msdn.microsoft.com/en-us/library/8cxs58a6%28v=VS.90%29.aspx .)

It works either '-' or '/'.  Most linker parameters that we uses are '-'

link -NOLOGO -DLL -OUT:xul.dll -PDB:xul.pdb -SUBSYSTEM:WINDOWS  dlldeps-xul.obj
nsStaticXULComponents.obj nsDllMain.obj dlldeps.obj nsGFXDeps.obj dlldeps-zlib.o
bj nsUnicharUtils.obj nsBidiUtils.obj nsRDFResource.obj    ./module.res -NXCOMPA
T -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV -MANIFEST:NO -LIBPATH:"c:/Workspac
e/hg.mozilla.org/objdir-win32-1/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLI
B:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd ...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, though, maybe this patch should be changing all the locations where we currently have this code:
http://mxr.mozilla.org/mozilla-central/search?string=HEAP%3A0
(r=dbaron and a=dbaron stand with that change)

Comment 12

[Makoto Kato [:m_kato]]

Attachment: patch for check in

Comment 13

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/mozilla-central/rev/4e8a688ab358

Comment 14

[Makoto Kato [:m_kato]]

We may need this for xpcshell.  xpcshell test hang (timeout) is same reason (stack overflow).

Comment 15

[Makoto Kato [:m_kato]]

Attachment: for xpcshell

fix xpcshell test hang (I forget xpcshell for test!)

Comment 16

[Makoto Kato [:m_kato]]

land for xpcshell
http://hg.mozilla.org/mozilla-central/rev/dc804317e500

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And xpcshell tests are now green.