Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(208)

Issue 1514083002: Fix intermittent GrAutoLocaleSetter crashes on Windows (Closed)

Created:
5 years ago by Kimmo Kinnunen
Modified:
5 years ago
Reviewers:
bsalomon
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Try to fix intermittent GrAutoLocaleSetter crashes on Windows Try to fix intermittent GrAutoLocaleSetter crashes on Windows. The string returned by setlocale() is only valid up to next setlocale(), so it can not be fed to setlocale(). Also, libraries such as ANGLE might call setlocale when inside a callstack containing GrAutoLocaleSetter. This would render the old locale pointer invalid. Committed: https://skia.googlesource.com/skia/+/aab16e54cb6ab712b477ed9eed8b9aa52098572c

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : address review comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -5 lines) Patch
M src/gpu/GrAutoLocaleSetter.h View 1 2 4 chunks +16 lines, -5 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
Kimmo Kinnunen
I could not repro the crash, though. http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-HD4600-x86_64-Debug-ANGLE/builds/1578/steps/dm/logs/stdio
5 years ago (2015-12-10 13:21:44 UTC) #3
Kimmo Kinnunen
https://msdn.microsoft.com/en-us/library/x99tb11d.aspx
5 years ago (2015-12-10 13:26:05 UTC) #5
bsalomon
https://codereview.chromium.org/1514083002/diff/20001/src/gpu/GrAutoLocaleSetter.h File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1514083002/diff/20001/src/gpu/GrAutoLocaleSetter.h#newcode49 src/gpu/GrAutoLocaleSetter.h:49: if (!fOldLocale.isEmpty()) { IIUC empty string is valid. "If ...
5 years ago (2015-12-10 15:12:24 UTC) #6
Kimmo Kinnunen
https://codereview.chromium.org/1514083002/diff/20001/src/gpu/GrAutoLocaleSetter.h File src/gpu/GrAutoLocaleSetter.h (right): https://codereview.chromium.org/1514083002/diff/20001/src/gpu/GrAutoLocaleSetter.h#newcode49 src/gpu/GrAutoLocaleSetter.h:49: if (!fOldLocale.isEmpty()) { On 2015/12/10 15:12:23, bsalomon wrote: > ...
5 years ago (2015-12-11 07:50:17 UTC) #7
bsalomon
lgtm
5 years ago (2015-12-11 13:59:32 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514083002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514083002/40001
5 years ago (2015-12-14 06:58:45 UTC) #10
commit-bot: I haz the power
5 years ago (2015-12-14 07:12:35 UTC) #12
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/aab16e54cb6ab712b477ed9eed8b9aa52098572c

Powered by Google App Engine
This is Rietveld 408576698