Bug 130304 - Remove all uses of deprecatedCharacters from JavaScriptCore
Summary: Remove all uses of deprecatedCharacters from JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks: 126854
  Show dependency treegraph
 
Reported: 2014-03-16 09:40 PDT by Darin Adler
Modified: 2014-03-17 00:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (27.34 KB, patch)
2014-03-16 09:52 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (27.26 KB, patch)
2014-03-16 10:33 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2014-03-16 09:40:59 PDT
Remove all uses of deprecatedCharacters from JavaScriptCore
Comment 1 Darin Adler 2014-03-16 09:52:47 PDT
Created attachment 226842 [details]
Patch
Comment 2 Anders Carlsson 2014-03-16 10:32:55 PDT
Comment on attachment 226842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226842&action=review

> Source/JavaScriptCore/API/JSValueRef.cpp:336
> +    if (str.is8Bit()) {

I think this will crash if str is a null string.
Comment 3 Darin Adler 2014-03-16 10:33:50 PDT
Created attachment 226843 [details]
Patch
Comment 4 Darin Adler 2014-03-16 10:36:19 PDT
Committed r165703: <http://trac.webkit.org/changeset/165703>
Comment 5 Andreas Kling 2014-03-16 22:43:28 PDT
Looks like this broke JSC tests on bots:

1   0x109b29a9e WTF::StringImpl::destroy(WTF::StringImpl*)
2   0x10998ea0c JSStringRelease
3   0x10999fa31 -[JSValue valueForProperty:]
4   0x109725741 __testObjectiveCAPI_block_invoke354
5   0x10994b45d -[JSContext(Internal) valueFromNotifyException:]
6   0x10994abba -[JSContext evaluateScript:withSourceURL:]
7   0x10971ebe2 testObjectiveCAPI
8   0x10971651c main
9   0x7fff8b5645fd start
10  0x1
Comment 6 Andreas Kling 2014-03-16 23:18:28 PDT
(In reply to comment #5)
> Looks like this broke JSC tests on bots:
> 
> 1   0x109b29a9e WTF::StringImpl::destroy(WTF::StringImpl*)
> 2   0x10998ea0c JSStringRelease
> 3   0x10999fa31 -[JSValue valueForProperty:]
> 4   0x109725741 __testObjectiveCAPI_block_invoke354
> 5   0x10994b45d -[JSContext(Internal) valueFromNotifyException:]
> 6   0x10994abba -[JSContext evaluateScript:withSourceURL:]
> 7   0x10971ebe2 testObjectiveCAPI
> 8   0x10971651c main
> 9   0x7fff8b5645fd start
> 10  0x1

Fixed this in <http://trac.webkit.org/r165719>. Not sure why webkit-patch added an r=andersca.
Comment 7 Darin Adler 2014-03-17 00:24:03 PDT
Thanks Andreas, for fixing my mistake.

Maybe someone who understands the rationale should add a comment explaining why we go out of our way to make a new string instead of using the existing one.