Closed
Bug 1181142
Opened 9 years ago
Closed 8 years ago
crash in strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: philipp, Assigned: erahm)
References
Details
(Keywords: crash, topcrash-win)
Crash Data
Attachments
(5 files, 3 obsolete files)
1.70 KB,
patch
|
glandium
:
review+
erahm
:
checkin+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
3.30 KB,
text/plain
|
Details | |
23.83 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
24.39 KB,
patch
|
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-f04217f0-990d-4b5b-93c0-fc4092150707. ============================================================= filing this bug because there is no associated report to that signature yet. some more information about the circumstances of this crash are available in a thread at https://support.mozilla.org/en-US/questions/1070827 where the user posted about this problem.
(Don't trust the stack for this one, bug 1181148) The strcmp fell off the end of a page: if (strcmp(result, "(intermediate value)")) |result| is 0x29d1fffe and its memory mapping stops at 0x29d20000. The CRT is trying to be clever and compare four bytes at a time, since the length of the string is 0x15. Is it legit for |result| to have such a value? If so then I guess we'll need to work around this.
Flags: needinfo?(luke)
Comment 2•9 years ago
|
||
It looks like 'result' ends up receiving memory malloc'd by js_malloc (which I think is just the global malloc). Is '*result' == 0? If I'm reading you correctly, that would be the only valid input. If so, I wonder if the builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're breaking that assumption with jemalloc?
Flags: needinfo?(luke)
> Is '*result' == 0? If I'm reading you > correctly, that would be the only valid input. Not sure I understand. As far as I can tell, either |result| is a 1-byte allocation consisting of just a null-terminator, or a 2-byte allocation consisting of any random char followed by a null. > If so, I wonder if the > builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're > breaking that assumption with jemalloc? Yeah, could be.
Comment 4•9 years ago
|
||
Oops, I was thinking of 'jschar' (two bytes), but I see this is a normal 'char'. Ok, so it could be a 0 or 1-length null-terminated string.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2) > It looks like 'result' ends up receiving memory malloc'd by js_malloc (which > I think is just the global malloc). Is '*result' == 0? If I'm reading you > correctly, that would be the only valid input. If so, I wonder if the > builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're > breaking that assumption with jemalloc? On linux and OS X, bug 691003 modified the behavior to make the minimum alignment 4 bytes (8 on 64-bit). We decided not to do Windows, but I wonder if we should. In this case we could do something more clever: return the size of result from |DecompileExpressionFromStack| and then directly use memcmp (not strcmp) iff size == 15.
Comment 6•9 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #5) > On linux and OS X, bug 691003 modified the behavior to make the minimum > alignment 4 bytes (8 on 64-bit). We decided not to do Windows, but I wonder > if we should. If we can confirm it is breaking widespread msvcrt assumptions, that might be the right fix. > In this case we could do something more clever: return the size of result > from |DecompileExpressionFromStack| and then directly use memcmp (not > strcmp) iff size == 15. Patches welcome, but this is pretty cold code :)
Bug 691003 comment 0: > The Linux ABI specifies that memory returned by malloc and friends is word > aligned at minimum. (4 bytes on 32-bit, 8 bytes on 64-bit.) > > GCC knows this, and its value range propagation pass will carry this > information around from malloc and company to things like strlen or strcpy. > > When gcc decides to inline the string functions, if it knows that the memory > they're operating on is word aligned, it will skip the usual byte-at-a-time > preamble and start with word-at-a-time accesses. > > This will cause a crash if you allocate e.g. two bytes and jemalloc decides > to place them in the last two bytes of a page before an unmapped region. Gee, sounds familiar :)
Assignee | ||
Comment 8•9 years ago
|
||
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we now need to do this on Windows as well.
Attachment #8630719 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment 9•9 years ago
|
||
Comment on attachment 8630719 [details] [diff] [review] Make the minumum allocation size word sized on all platforms Review of attachment 8630719 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozjemalloc/jemalloc.c @@ +551,1 @@ > #define TINY_MIN_2POW (sizeof(void*) == 8 ? 3 : 2) If we're worried about the compiler assuming things based on what its associated CRT does, then this isn't going to cut it: from https://msdn.microsoft.com/en-us/library/6ewkz86d%28v=vs.120%29.aspx: "(In Visual C++, the fundamental alignment is the alignment that's required for a double, or 8 bytes. In code that targets 64-bit platforms, it’s 16 bytes)" The "fun" part is that this is likely to regress our memory usage quite severely (iirc from jemalloc3, which didn't have the same minimum size). The same change should be done to jemalloc3 too.
Attachment #8630719 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2114b86181
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c65e7f351a
Assignee | ||
Comment 12•9 years ago
|
||
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we now need to do a similar change on Windows as well. For Windows the requirement is double-aligned, so 8-bytes on 32-bit and 16-bytes on 64-bit.
Assignee | ||
Updated•9 years ago
|
Attachment #8630719 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
I've updated the patch to use 8/16 bytes on Windows. I have two talos runs pending: - baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c65e7f351a - w/ patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2114b86181 I will be out for the rest of the week, so if someone else wants to finish this off before I get back they are more than welcome to.
Assignee | ||
Comment 14•9 years ago
|
||
(as glandium said, we'll need to update jemalloc3 as well)
Comment 15•9 years ago
|
||
Comment on attachment 8630810 [details] [diff] [review] Make the minumum allocation size word sized on all platforms Review of attachment 8630810 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozjemalloc/jemalloc.c @@ +545,5 @@ > #define CACHELINE ((size_t)(1U << CACHELINE_2POW)) > > /* > + * Smallest size class to support. On Windows the smallest allocation size > + * must be the size of a double (8 bytes on 32-bit, 16 bytes on 64-bit). On I would hope that a double is still 8 bytes on 64-bit! I suspect the reason for 16-byte alignment is to accommodate some other type. Maybe just say "must be 8 bytes on 32-bit, 16 bytes on 64-bit"?
Comment 16•9 years ago
|
||
Do we really need to go to 8/16 on Windows? I know that's what the docs say but the problematic case is 1 or 2 bytes. In theory Linux requires 8/16 as well (see the start of http://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html) but 4/8 is working fine.
Comment 17•9 years ago
|
||
What matters is not what glibc claims it does, but what POSIX guarantees, which is, according to http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html that "The pointer returned if the allocation succeeds shall be suitably aligned so that it may be assigned to a pointer to any type of object and then used to access such an object in the space allocated". That means 4/8. I doubt gcc/clang will optimize based on what glibc actually does, as opposed to what POSIX says, especially considering the number of different platforms they target. But more than conjecture, we can actually look at their source and know for sure. OTOH, MSVC is pretty much tied to its CRT, and it may be doing much more assumptions about the CRT implementation than GCC is doing. Which we can't know since we don't have its source.
Comment 18•9 years ago
|
||
Frankly I suspect we'd be perfectly fine with 4/8 on Windows. We're worrying about a potential future bug that probably doesn't exist. I tried to bring this up at some point (IRC maybe? I don't remember) and it didn't lead anywhere and I don't care enough to push on it further.
Comment 19•9 years ago
|
||
Eric's try results seem to indicate there is no RSS regressions from the change to 8/16, so I'd rather go with security. OTOH, our memory testing on Windows is fairly limited :( I wish we had AWSY for Windows.
Comment 20•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #19) > Eric's try results seem to indicate there is no RSS regressions from the > change to 8/16, so I'd rather go with security. OTOH, our memory testing on > Windows is fairly limited :( I wish we had AWSY for Windows. Can we approximate by testing 8/16 on Linux?
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #20) > (In reply to Mike Hommey [:glandium] from comment #19) > > Eric's try results seem to indicate there is no RSS regressions from the > > change to 8/16, so I'd rather go with security. OTOH, our memory testing on > > Windows is fairly limited :( I wish we had AWSY for Windows. > > Can we approximate by testing 8/16 on Linux? It won't be a perfect comparison, but I can certainly do a run.
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8e51984e3a
Assignee | ||
Comment 23•9 years ago
|
||
https://areweslimyet.com/?series=erahm_bug_1181142&evenspacing Some looks better, some looks worse, roughly it's all in the noise. The RSS after TP5 is probably the most interesting, we see a 18.68MiB increase (~5%), but after 30 seconds we drop back down and only see a ~1% increase. Also note this was a 64-bit build (that's all AWSY supports), I'd expect less of an effect on a 32-bit build. Given the relatively clean talos run it seems like doing the change as-is would probably be fine.
Updated•9 years ago
|
Crash Signature: [@ strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)] → [@ strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)]
[@ strncpy | js::DecompileValueGenerator]
Comment 24•8 years ago
|
||
This signature is fairly high (#23 currently) in Top Crashers for Firefox 45.0. What are the next steps here?
Flags: needinfo?(erahm)
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #24) > This signature is fairly high (#23 currently) in Top Crashers for Firefox > 45.0. > What are the next steps here? I think it's reasonable to land this as-is, at this point I'm not inclined to pursue a jemalloc4 upstream follow up. Maybe we can split that off to a separate bug and get this landed? :glandium, do you agree?
Flags: needinfo?(erahm) → needinfo?(mh+mozilla)
Comment 26•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #25) > (In reply to Mats Palmgren (:mats) from comment #24) > > This signature is fairly high (#23 currently) in Top Crashers for Firefox > > 45.0. > > What are the next steps here? > > I think it's reasonable to land this as-is, at this point I'm not inclined > to pursue a jemalloc4 upstream follow up. Maybe we can split that off to a > separate bug and get this landed? > > :glandium, do you agree? jemalloc4 actually now has a default lg-tiny-min of 3 on all platforms, and it has a configure flag to change it. So if we want to do the same on both, we can either make mozjemalloc use the same value on both win32 and win64, or make jemalloc4 use 4 on win64.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 27•8 years ago
|
||
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we now need to do a similar change on Windows as well. For Windows the requirement is 8-bytes on 32-bit and 16-bytes on 64-bit. It's not clear to me how we would conditionally set the 'lg-tiny-min' flag to 4 for Win64 in jemalloc4 so I've left that out.
Attachment #8734028 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8630810 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8734028 -
Flags: review?(mh+mozilla) → review+
Comment 28•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #27) > It's not clear to me how we would conditionally set the 'lg-tiny-min' flag > to 4 > for Win64 in jemalloc4 so I've left that out. Something like: if test -n "$HAVE_64BIT_BUILD"; then ac_configure_args="$ac_configure_args --with-lg-tiny-min=4" else ac_configure_args="$ac_configure_args --with-lg-tiny-min=3" fi in build/autoconf/jemalloc.m4
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8734558 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4593c32d5b81c5e2f2cd1332a7e49c3fc81cbcb0 Bug 1181142 - Part 1: Make the minimum allocation size word sized on all platforms. r=glandium
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Updated•8 years ago
|
Attachment #8734028 -
Flags: checkin+
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4593c32d5b81
Comment 32•8 years ago
|
||
Backed out for xpcshell failure in test_dmd.js on Windows 8 x64 debug. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c49c1cfcbc7c Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4593c32d5b81 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=24617197&repo=mozilla-inbound 19:00:31 WARNING - TEST-UNEXPECTED-FAIL | memory/replace/dmd/test/test_dmd.js | test - [test : 113] full-unsampled1-live - false == true 19:00:31 INFO - C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:test:113 19:00:31 INFO - C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:test2:145 19:00:31 INFO - C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:run_test:155 19:00:31 INFO - C:\slave\test\build\tests\xpcshell\head.js:_execute_test:528
Flags: needinfo?(erahm)
Assignee | ||
Comment 33•8 years ago
|
||
So the DMD test is upset because the minimum allocation is now greater than 8 on windows 64. DMD is fine, the test is just rather fragile. If I modify the expected results the test will fail on all other builds. We could possibly update the test to only malloc 16-bytes and higher. I'm not sure the best way forward here, after looking at the test I'm not clear on how it works at all (lots of dummy functions, something about analyze 1 and analyze 2 specified with aNum, etc). njn, I think you have pending changes for these tests in another bug. Do you think you could take a look at what needs to be changed after that lands to handle the minimum size being 16-bytes on Windows 64?
Flags: needinfo?(erahm) → needinfo?(n.nethercote)
Assignee | ||
Comment 34•8 years ago
|
||
This is the diff of the expected and actual output. You can see all the percentages are messed up and the 8 byte allocation has 8 bytes of slop instead of 0 (causing all the other calculations to be off).
Comment 35•8 years ago
|
||
I'm happy to make the relevant changes in bug 1253512's patches. (The changes in that bug make the required changes for 16 byte allocations much simpler; score one point for getting rid of "sampling".) I'm planning to land those patches on Tuesday (Australian time); I won't get to it before then because of the Easter long weekend here. Sorry about that; hopefully a few extra days won't matter in the long run.
Flags: needinfo?(n.nethercote)
Updated•8 years ago
|
Attachment #8734558 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 36•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #35) > I'm happy to make the relevant changes in bug 1253512's patches. (The > changes in that bug make the required changes for 16 byte allocations much > simpler; score one point for getting rid of "sampling".) > > I'm planning to land those patches on Tuesday (Australian time); I won't get > to it before then because of the Easter long weekend here. Sorry about that; > hopefully a few extra days won't matter in the long run. Thank you, that should be fine. I think we just want to land it before the next uplift on April 18th.
Comment 37•8 years ago
|
||
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/c49c1cfcbc7c
Comment 38•8 years ago
|
||
I'm not especially familiar with mozjemalloc, so this might be totally off-base, but is there a reason you can't just say `#define TINY_MIN_2POW alignof(max_align_t)` now? (This applies to OS X/Linux as well.) That should guarantee the minimum alignment is set properly on all platforms. I imagine the reason no one used alignof in bug 691003 is that C11 hadn't been ratified by the ISO at the time it landed.
Comment 39•8 years ago
|
||
(In reply to Jim Porter (:squib) from comment #38) > I'm not especially familiar with mozjemalloc, so this might be totally > off-base, but is there a reason you can't just say `#define TINY_MIN_2POW > alignof(max_align_t)` now? (This applies to OS X/Linux as well.) That should > guarantee the minimum alignment is set properly on all platforms. I suppose you'd have to take the base-2 log of this value, actually. That's probably moderately annoying to do at compile time in C...
Comment 40•8 years ago
|
||
We don't have alignof support in all supported compilers. https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code But mfbt/TemplateLib.h would have CeilingLog2<N>::value for the purpose, if alignof existed. (Although of course there's about zero chance of that being upstreamable unless jemalloc becomes C++.)
Comment 41•8 years ago
|
||
There's a dev-platform thread about switching to MSVC 2015, and the thread about deprecating OS X 10.6-8 says we're doing 10.6 builds with a dev version of clang 3.8, so perhaps we're close to being able to use alignof? If my reading is accurate, perhaps a followup bug to use alignof would be in order?
Assignee | ||
Comment 42•8 years ago
|
||
(In reply to Jim Porter (:squib) from comment #41) > There's a dev-platform thread about switching to MSVC 2015, and the thread > about deprecating OS X 10.6-8 says we're doing 10.6 builds with a dev > version of clang 3.8, so perhaps we're close to being able to use alignof? > If my reading is accurate, perhaps a followup bug to use alignof would be in > order? Feel free to file a follow up. It looks like |alignof| has been around since MSVC 2012 so we probably don't have to wait for the 2015 switch. We'll still want to do something that degrades reasonably on non-tier-1 platforms that may not support alignof (or std::max_align_t). For upstream jemalloc (jemalloc 4 as of this writing) they use a config flag rather than a define, so it's not really worth discussing the upstreamability. We'll also need to measure potential regressions on Linux/OSX with such a change (most likely via Talos). Also note for OSX, in theory, the minimum size is 16-bytes on both 32-bit and 64-bit targets. I'm not sure what |alignof(max_align_t)| would end up being on those platforms, nor am I sure if we'd want to take any regressions without a compelling reason to change (such as crashes).
Comment 43•8 years ago
|
||
Well, if there are cases where we don't want to use `alignof(max_align_t)`, I think it'd be reasonable to add those in, provided we fall back to using alignof as a default. (And possibly allow using a config option for platforms that don't support alignof.) I'll file a followup in a moment.
Comment 44•8 years ago
|
||
Note the smallest version of clang we support building with is still 3.3, and the smallest version we actually build with on automation is 3.5. But anyways, is using `alignof(max_align_t)` really worth doing? What we have after this bug landed is close enough, and doesn't need to go through hoops to get the log2 of its result at compile time (and compiling jemalloc.c as C++ *is* going through hoops)
Comment 45•8 years ago
|
||
Having looked at the rest of the source file, it looks like it would be easy to just change TINY_MIN_2POW to be the alignment size rather than log2 of the alignment size. All but one instance of TINY_MIN_2POW uses it with bit-shifting operators, and they could be replaced with multiplication or division; an optimizing compiler should preserve performance in those cases. Here's the only exception I see: <https://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#1130>.
Comment 46•8 years ago
|
||
erahm, I will assume that you will land this patch along with the other ones, and therefore I don't need to do anything more here. Please let me know if that's not the case. Thank you.
Attachment #8735718 -
Flags: review?(erahm)
Updated•8 years ago
|
Assignee: erahm → n.nethercote
Updated•8 years ago
|
Assignee: n.nethercote → erahm
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8735718 [details] [diff] [review] Update DMD tests to handle a minimum alloc size of 16 on Win64 Review of attachment 8735718 [details] [diff] [review]: ----------------------------------------------------------------- r=me Thank you for updating the tests. I'll land this along with the other two patches.
Attachment #8735718 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 48•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1208dc36a238
Assignee | ||
Comment 49•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b78203d8b153d16708b6488f30f131bbe8c8f0 Bug 1181142 - Part 0: Update DMD tests to handle a minimum alloc size of 16 on Win64. r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/45c01e7fbe3fb034e0a2b5bfb6a6a745b19497ff Bug 1181142 - Part 1: Make the minimum allocation size word sized on all platforms. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/0a14d675236ec1f37d1251b7e9ea5f9aa2aea35e Bug 1181142 - Part 2: Make the minimum jemalloc4 allocation size 16 bytes on Windows 64. r=glandium
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 50•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77b78203d8b1 https://hg.mozilla.org/mozilla-central/rev/45c01e7fbe3f https://hg.mozilla.org/mozilla-central/rev/0a14d675236e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 51•8 years ago
|
||
This crash is #19 in top-crash for 45.0.2 and #25 for beta. It increased by ~36% in 45.0.2. Should we uplift the patch in 47 ?
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
Flags: needinfo?(erahm)
Keywords: topcrash-win
Assignee | ||
Comment 52•8 years ago
|
||
Just to be clear, 47 is beta now right? I'll probably have to rebase part 0 (there was some churn in DMD outside of this bug), I imagine part 1 will apply cleanly. We can skip 2.
Flags: needinfo?(erahm)
Assignee | ||
Comment 53•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b3e160c29d7
Assignee | ||
Comment 54•8 years ago
|
||
This is the roll-up for beta if we want to take it. Try run is pending.
Comment 55•8 years ago
|
||
yep 47 is beta
Assignee | ||
Comment 56•8 years ago
|
||
Comment on attachment 8745491 [details] [diff] [review] Make the minimum allocation size word sized on all platforms Note: This patch is a rebased roll-up of Part 0 and Part 1. Approval Request Comment [Feature/regressing bug #]: None. [User impact if declined]: Top crasher keeps crashing. [Describe test coverage new/current, TreeHerder]: Baked on nightly, now aurora. [Risks and why]: None. [String/UUID change made/needed]: None.
Attachment #8745491 -
Flags: approval-mozilla-beta?
Comment on attachment 8745491 [details] [diff] [review] Make the minimum allocation size word sized on all platforms Crash fix, there was no crash data for Fx48 that I could use to verify, it's still worth uplifting. Beta47+
Attachment #8745491 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 58•8 years ago
|
||
Okay it looks like I need to update the DMD test further, lets hold off on uplifting.
Hi Eric, any updates on when we are ready to uplift this to Beta? Thanks!
Flags: needinfo?(erahm)
Assignee | ||
Comment 60•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #59) > Hi Eric, any updates on when we are ready to uplift this to Beta? Thanks! I'll see what I can do today, I need to figure out how to run the tests locally on the beta branch.
Flags: needinfo?(erahm)
Assignee | ||
Comment 61•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f01ec5e96f
Assignee | ||
Comment 62•8 years ago
|
||
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we now need to do a similar change on Windows as well. For Windows the requirement is 8-bytes on 32-bit and 16-bytes on 64-bit.
Assignee | ||
Updated•8 years ago
|
Attachment #8745491 -
Attachment is obsolete: true
Assignee | ||
Comment 63•8 years ago
|
||
Comment on attachment 8749777 [details] [diff] [review] Make the minimum allocation size word sized on all platforms This updates the tests to pass with the latest code. The previous version was already approved. See comment 57, comment 56. Ritu can you approve this again, we should be good to land now.
Flags: needinfo?(rkothari)
Attachment #8749777 -
Flags: approval-mozilla-beta?
Comment on attachment 8749777 [details] [diff] [review] Make the minimum allocation size word sized on all platforms Crash fix, Beta47+
Flags: needinfo?(rkothari)
Attachment #8749777 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 65•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/127f13817e6f
You need to log in
before you can comment on or make changes to this bug.
Description
•