Bug 127161

Summary: Get rid of OpaqueJSString::deprecatedCharacters()
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: New BugsAssignee: Anders Carlsson <andersca>
Severity: Normal CC: commit-queue, ggaren, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 127164    
Bug Blocks:    
Description Flags
Patch sam: review+

Description Anders Carlsson 2014-01-16 22:04:55 PST
Get rid of OpaqueJSString::deprecatedCharacters()
Comment 1 Anders Carlsson 2014-01-16 22:29:40 PST
Created attachment 221440 [details]
Comment 2 Sam Weinig 2014-01-16 22:34:58 PST
Comment on attachment 221440 [details]

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

> Source/JavaScriptCore/API/OpaqueJSString.cpp:45
> +    if (UChar* characters = m_characters)
> +        fastFree(static_cast<void*>(characters));

You should add a comment about why you are putting this in a local.

> Source/JavaScriptCore/API/OpaqueJSString.cpp:78
> +    UChar* characters = m_characters;
> +    if (characters)
> +        return characters;

Same here.
Comment 3 Anders Carlsson 2014-01-16 22:46:20 PST
Committed r162185: <http://trac.webkit.org/changeset/162185>
Comment 4 Alexey Proskuryakov 2014-01-16 22:58:30 PST
This broke Windows build:

     1>c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\X86Assembler.h(2348): warning C4309: 'argument' : truncation of constant value
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(97): error C2039: 'atomic' : is not a member of 'std'
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(97): error C2143: syntax error : missing ';' before '<'
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(97): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(97): error C2238: unexpected token(s) preceding ';'
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(79): error C2614: 'OpaqueJSString' : illegal member initialization: 'm_characters' is not a base or member
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(85): error C2614: 'OpaqueJSString' : illegal member initialization: 'm_characters' is not a base or member
     1>c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(91): error C2614: 'OpaqueJSString' : illegal member initialization: 'm_characters' is not a base or member
Comment 5 Alexey Proskuryakov 2014-01-16 23:04:25 PST
And Mac too:

Undefined symbols for architecture x86_64:
  "__ZN14OpaqueJSStringD1Ev", referenced from:
      __ZN3WTF20ThreadSafeRefCountedI14OpaqueJSStringE5derefEv in ScriptCachedFrameData.o
      __ZN3WTF20ThreadSafeRefCountedI14OpaqueJSStringE5derefEv in ScriptController.o

Undefined symbols for architecture i386:
  "__ZN14OpaqueJSStringD1Ev", referenced from:
      __ZN24OpaqueJSClassContextDataD2Ev in ScriptCachedFrameData.o
Comment 6 Alexey Proskuryakov 2014-01-16 23:59:48 PST
...and it also fails JSC API test:

2014-01-16 23:47:39.670 testapi[96811:507] myObj2.currentThis: [object Object]
ASSERTION FAILED: JSStringGetCharactersPtr(constantStringRef) == constantString
/Volumes/Data/slave/mavericks-release/build/Source/JavaScriptCore/API/tests/testapi.c(1263) : int main(int, char **)
1   0x7fff8e72a5fd start
2   0x1

Comment 7 WebKit Commit Bot 2014-01-17 00:08:01 PST
Re-opened since this is blocked by bug 127164
Comment 8 Alexey Proskuryakov 2014-01-17 00:09:55 PST
Looks like this really breaks semantics of JSStringCreateWithCharactersNoCopy, rolling out :(
Comment 9 Anders Carlsson 2014-01-17 09:06:26 PST
Created attachment 221471 [details]
Comment 10 Anders Carlsson 2014-01-17 09:12:29 PST
Created attachment 221472 [details]
Comment 11 Anders Carlsson 2014-01-17 09:40:42 PST
Committed r162205: <http://trac.webkit.org/changeset/162205>
Comment 12 Alexey Proskuryakov 2014-01-17 09:41:44 PST
Comment on attachment 221472 [details]

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

> Source/JavaScriptCore/API/OpaqueJSString.h:98
> +    // This will be initialized on demand when characters() is called.

This comment seems off now.
Comment 13 Alexey Proskuryakov 2014-01-17 09:45:17 PST
And this broke build on Windows (EWS also said so):

     1>C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xxatomic(229): error C2664: 'std::_Atomic_address &std::_Atomic_address::operator =(const std::_Atomic_address &)' : cannot convert argument 1 from 'const UChar *' to 'void *'
                 Conversion loses qualifiers
                 C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xxatomic(227) : while compiling class template member function 'std::atomic<const UChar *>::atomic(_Ty *) throw()'
                     _Ty=const UChar
                 c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(75) : see reference to function template instantiation 'std::atomic<const UChar *>::atomic(_Ty *) throw()' being compiled
                     _Ty=const UChar
                 c:\cygwin\home\buildbot\slave\win-release\build\source\javascriptcore\api\OpaqueJSString.h(99) : see reference to class template instantiation 'std::atomic<const UChar *>' being compiled