Last Comment Bug 700822 - Land hardening option and win32 ExecutableAllocator randomization
: Land hardening option and win32 ExecutableAllocator randomization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on: 728623 960416 1232907
Blocks: JITHardening
  Show dependency treegraph
 
Reported: 2011-11-08 14:55 PST by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2015-12-15 20:37 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Add explanatory comment. (888 bytes, patch)
2012-02-24 11:11 PST, Chris Leary [:cdleary] (not checking bugmail)
bzbarsky: review+
Details | Diff | Review

Description Chris Leary [:cdleary] (not checking bugmail) 2011-11-08 14:55:19 PST
Separate bug for landing data/resolution, based on patch 2 in bug 677272 (which I should have split into different bugs to begin with).

It's been rebased, pushed to try:

https://hg.mozilla.org/try/pushloghtml?changeset=a19f0237eb77
Comment 3 Marco Bonardo [::mak] 2011-11-10 03:20:58 PST
ehr 1bf4c1a6412b has been backed out
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 08:51:26 PST
Yeah, sorry about that -- I thought I had added a backout cset comment, but apparently not. Have to debug the frequent WinXP reftest failure caused by the win32 randomization patch today.
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-11-14 11:21:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea9d4ad53a0 (backout)

Need to debug failing Windows XP reftest.
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2012-02-14 11:48:01 PST
Take 2, with WinXP workaround:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e93f190f64c

It looked good on try, hopefully it sticks.
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2012-02-14 11:49:00 PST
dvander gave post-push r+ on the workaround, because I felt like I could use a sanity check. Thanks, David!
Comment 8 Marco Bonardo [::mak] 2012-02-15 09:02:02 PST
https://hg.mozilla.org/mozilla-central/rev/9e93f190f64c
Comment 9 Boris Zbarsky [:bz] 2012-02-18 18:56:28 PST
Why do we need the separate RandomizeIsBrokenImpl and RandomizeIsBroken?
Comment 10 Alex Keybl [:akeybl] 2012-02-23 13:26:53 PST
It's not clear why we need to track this for FF11. Please re-nominate if you disagree with this bug being untracked for FF11.
Comment 11 Chris Leary [:cdleary] (not checking bugmail) 2012-02-23 13:58:59 PST
(In reply to Boris Zbarsky (:bz) from comment #9)
> Why do we need the separate RandomizeIsBrokenImpl and RandomizeIsBroken?

I was trying to get the compiler intrinsic guards for |static type val = expr()| to avoid calling RandomizeIsBroken twice if you race to create runtimes from different threads. Kind of a hack, but I think it should work out okay. I can file a followup bug if you think that's too heinous.
Comment 12 Boris Zbarsky [:bz] 2012-02-23 14:05:24 PST
I thinks it's OK for that, assuming the compiler handles such a race correctly, but then you need a nice comment explaining that's what's going on!
Comment 13 Chris Leary [:cdleary] (not checking bugmail) 2012-02-23 14:09:54 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> I thinks it's OK for that, assuming the compiler handles such a race
> correctly, but then you need a nice comment explaining that's what's going
> on!

Yeah, mea culpa. Will pastebin r? you on IRC in a little while for a comment patch. :-)
Comment 14 Chris Leary [:cdleary] (not checking bugmail) 2012-02-24 11:11:44 PST
Created attachment 600451 [details] [diff] [review]
Add explanatory comment.
Comment 15 Boris Zbarsky [:bz] 2012-02-24 11:13:24 PST
Comment on attachment 600451 [details] [diff] [review]
Add explanatory comment.

r=me
Comment 16 Boris Zbarsky [:bz] 2012-02-24 11:18:56 PST
Though are you sure that all our compilers do threadsafety stuff here?
Comment 17 Chris Leary [:cdleary] (not checking bugmail) 2012-02-24 11:19:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/941481edc290
Comment 18 Chris Leary [:cdleary] (not checking bugmail) 2012-02-24 11:23:32 PST
(In reply to Boris Zbarsky (:bz) from comment #16)
> Though are you sure that all our compilers do threadsafety stuff here?

I'm not totally sure about MSVC, but, even if they do race, they're writing the same atomic-sized value (int) to the static, so you should get consistency anyway. Kind of hacky, but I'm not sure we even really support this mutliple runtimes racing for creation on multiple threads use case anyway, it's kind of a best-effort approach on my part for a simple thread-safe memoization.
Comment 19 Marco Bonardo [::mak] 2012-02-25 02:20:29 PST
https://hg.mozilla.org/mozilla-central/rev/941481edc290

Note You need to log in before you can comment on or make changes to this bug.