Closed Bug 1347796 Opened 3 years ago Closed 3 years ago

Win64 ASan TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_ObjectUtils.js

Categories

(Toolkit :: General, defect)

x86_64
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ting, Assigned: ting)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Stack overflow exception (not reported by ASan) in deepEqual() for the test case:

  https://dxr.mozilla.org/mozilla-central/rev/ff04d410e74b69acfab17ef7e73e7397602d5a68/toolkit/modules/tests/xpcshell/test_ObjectUtils.js#93-103

08:29:12     INFO -  TEST-START | toolkit/modules/tests/xpcshell/test_ObjectUtils.js
08:29:14  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | xpcshell return code: -1073741571
08:29:14     INFO -  TEST-INFO took 2632ms
08:29:14     INFO -  >>>>>>>
08:29:14     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
08:29:14     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
08:29:14     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
08:29:14     INFO -  running event loop
08:29:14     INFO -  toolkit/modules/tests/xpcshell/test_ObjectUtils.js | Starting test_deepEqual
08:29:14     INFO -  (xpcshell/head.js) | test test_deepEqual pending (2)
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 10] deepEqual date - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 11] deepEqual invalid dates - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 13] deepEqual date - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 16] Dates and times should not be equal - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 17] Times and dates should not be equal - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 19] Objects and primitives should not be equal - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 20] RegExps and strings should not be equal - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 21] Strings and RegExps should not be equal - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 24] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 25] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 26] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 27] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 28] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 29] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 30] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 31] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 32] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 33] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 37] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 40] deepEqual == check - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 41] deepEqual == check - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 42] deepEqual == check - true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 46] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 47] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 48] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 49] true == true
08:29:14     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 50] true == true
08:29:15     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 58] true == true
08:29:15     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 59] true == true
08:29:15     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 82] true == true
08:29:15     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 86] true == true
08:29:15     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 89] true == true
08:29:15     INFO -  <<<<<<<
ting@ting-nb:~/w/fx/tools/jsshell-linux64-asan$ ./js -f ~/w/fx/mc/js/xpconnect/tests/chrome/test_bug732665_meta.js 
Max stack size: 2063168 bytes 
Maximum number of frames: 116 
Average frame size: 17786 bytes

I am still trying to get the measurements by win64-asan jsshell...
(In reply to Ting-Yu Chou [:ting] from comment #2)
> So the size of trusted script buffer needs to be larger:

Enlarged kTrustedScriptBuffer to 1M but doesn't help, however the test passes if I set kSystemCodeBuffer 32K (not touching kTrustedScriptBuffer).
Attached patch js.cpp.diffSplinter Review
(In reply to Ting-Yu Chou [:ting] from comment #3)
> I am still trying to get the measurements by win64-asan jsshell...

With this attachment I measured:

C:\w\fx\mc\obj-asan\dist\bin>js.exe -f c:\w\fx\mc\js\xpconnect\tests\chrome\test_bug732665_meta.js
Max stack size: 2016736 bytes
Maximum number of frames: 108
Average frame size: 18674 bytes
The measurement from win64 opt:

C:\w\fx\tools\jsshell-win64>js.exe -f c:\w\fx\mc\js\xpconnect\tests\chrome\test_bug732665_meta.js
Max stack size: 1040192 bytes
Maximum number of frames: 117
Average frame size: 8891 bytes

So I think the original setting of kTrustedScriptBuffer for ASan (9.0 * 3 * 10 = 270k, 9.0 is average size per stack frame, 3 is 3x overhead) should be sufficient for "10 heavy stack frames deeper in privileged code". Bug 1067610 changed it to 9.0 * 5 * 10 = 450k, but not sure why. Enlarge kSystemCodeBuffer seems more reasonable.
(In reply to Ting-Yu Chou [:ting] from comment #3)
> ting@ting-nb:~/w/fx/tools/jsshell-linux64-asan$ ./js -f
> ~/w/fx/mc/js/xpconnect/tests/chrome/test_bug732665_meta.js 
> Max stack size: 2063168 bytes 
> Maximum number of frames: 116 
> Average frame size: 17786 bytes

Numbers of linux64-opt:

ting@ting-nb:~/w/fx/tools/jsshell-linux64-opt$ ./js -f ~/w/fx/mc/js/xpconnect/tests/chrome/test_bug732665_meta.js 
Max stack size: 1035536 bytes 
Maximum number of frames: 166 
Average frame size: 6239 bytes

So 3x stack overhead seems good enough based on the numbers here, and the numbers of win64-opt/win64-asan in comments above. It also matches to what Clang [1] specified:

  "AddressSanitizer uses more stack memory. We have seen up to 3x increase."

[1] https://clang.llvm.org/docs/AddressSanitizer.html
With:

  kSystemCodeBuffer = 36k,
  kTrustedScriptBuffer = 270k

the tests:

  js/xpconnect/tests/chrome/test_bug732665.xul,
  toolkit/modules/tests/xpcshell/test_ObjectUtils.js

are passed for win64-asan on my Windows 10.
Attached patch wip v1 (obsolete) — Splinter Review
Assignee: nobody → janus926
Status: NEW → ASSIGNED
(In reply to Ting-Yu Chou [:ting] from comment #6)
> The measurement from win64 opt:
> 
> C:\w\fx\tools\jsshell-win64>js.exe -f
> c:\w\fx\mc\js\xpconnect\tests\chrome\test_bug732665_meta.js
> Max stack size: 1040192 bytes
> Maximum number of frames: 117
> Average frame size: 8891 bytes

I'm not sure why the max stack size is ~1M for win64 opt, it should be 2M with this setting:

  https://dxr.mozilla.org/mozilla-central/rev/e1576dd8bd9d3a4ca418cf347133b8a4957ddeca/config/config.mk#417
(In reply to Ting-Yu Chou [:ting] from comment #10)
> I'm not sure why the max stack size is ~1M for win64 opt, it should be 2M
> with this setting:

It's because jsshell has different max stack size:

  https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/js/src/shell/js.cpp#137-141
Attachment #8848430 - Attachment is obsolete: true
Comment on attachment 8849424 [details]
Bug 1347796 - Bump stack size 3x larger than default for win64 ASan.

Redirecting to Shu who worked on this most recently.
Attachment #8849424 - Flags: review?(bobbyholley) → review?(shu)
Hardware: Unspecified → x86_64
See Also: → 1349153
The reason why linux64 ASan doesn't have the same issue is because the stack is 8MB (checked "ulimit -s" by TaskCluster one click loaner), which 2MB kStackQuota leaves plenty 6MB+10KB for the system code.  But win64 has 2MB stack, exactly 10KB is reserved.
Comment on attachment 8849424 [details]
Bug 1347796 - Bump stack size 3x larger than default for win64 ASan.

Cancel the review request. I now tend to enlarge the 2MB stack size in config.mk for MOZ_ASAN to fix both this and bug 1349153.
Attachment #8849424 - Flags: review?(shu)
Blocks: 1349153
Comment on attachment 8849424 [details]
Bug 1347796 - Bump stack size 3x larger than default for win64 ASan.

https://reviewboard.mozilla.org/r/122182/#review125216

r=dbaron, although I think a build peer should review as well

::: config/config.mk:417
(Diff revision 2)
>  endif # WINNT
>  
>  ifdef _MSC_VER
>  ifeq ($(CPU_ARCH),x86_64)
> +ifdef MOZ_ASAN
> +WIN32_EXE_LDFLAGS	+= -STACK:6291456

Maybe add a single-line comment saying that ASAN builds have 3x stack memory usage of normal builds?
Attachment #8849424 - Flags: review?(dbaron) → review+
Comment on attachment 8849424 [details]
Bug 1347796 - Bump stack size 3x larger than default for win64 ASan.

https://reviewboard.mozilla.org/r/122182/#review125216

Asked :gps for another review.

> Maybe add a single-line comment saying that ASAN builds have 3x stack memory usage of normal builds?

Added.
Comment on attachment 8849424 [details]
Bug 1347796 - Bump stack size 3x larger than default for win64 ASan.

https://reviewboard.mozilla.org/r/122182/#review126862

Seems reasonable.
Attachment #8849424 - Flags: review?(gps) → review+
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c1f4dd39496
Bump stack size 3x larger than default for win64 ASan. r=dbaron,gps
https://hg.mozilla.org/mozilla-central/rev/9c1f4dd39496
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.