Crash in conversion of empty OpaqueJSString to Identifier

[Allan Sandfeld Jensen]

KWallets integration QtWebKit causes crashes on entering a number of webpages.

The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString.

The problem seems to be the construction of an Identifier containing a null-string, which is invalid.

[Allan Sandfeld Jensen]

Created attachment 173518 [details]
Patch

[Simon Hausmann]

Comment on attachment 173518 [details]
Patch

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

> Source/JavaScriptCore/API/OpaqueJSString.cpp:54
>      if (!this || !m_string.length())
> -        return Identifier(globalData, static_cast<const char*>(0));
> +        return Identifier(Identifier::EmptyIdentifier);

I don't know this code well enough to feel comfortable reviewing the patch. It seems quite evil, but then again, this function is already checking for !this, so maybe this is okay.

[Michael Saboff]

(In reply to comment #0)
> KWallets integration QtWebKit causes crashes on entering a number of webpages.
> 
> The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString.
> 
> The problem seems to be the construction of an Identifier containing a null-string, which is invalid.

I think the change is fine, but could you provide a detailed stack trace?   Want to see if you should be getting to this place with a null OpaqueJSString in the first place.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (In reply to comment #0)
> > KWallets integration QtWebKit causes crashes on entering a number of webpages.
> > 
> > The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString.
> > 
> > The problem seems to be the construction of an Identifier containing a null-string, which is invalid.
> 
> I think the change is fine, but could you provide a detailed stack trace?   Want to see if you should be getting to this place with a null OpaqueJSString in the first place.

I don't think it is null, it is only empty. Note the check is "!m_string.length()".

[Geoffrey Garen]

Comment on attachment 173518 [details]
Patch

The old code tried to create a null string. The new code tries to create an empty string. This seems like a change in behavior.

Why should we change fundamental low-level behavior that has been this way for four years because of a crash in the Qt port?

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (From update of attachment 173518 [details])
> The old code tried to create a null string. The new code tries to create an empty string. This seems like a change in behavior.
> 
> Why should we change fundamental low-level behavior that has been this way for four years because of a crash in the Qt port?

Because the code is wrong?

[Allan Sandfeld Jensen]

I admit this bug report is too low in information for how low-level the change is. I will provide a detailed backtrace tomorrow, which should help to establish how general the problem is.

[Geoffrey Garen]

> Because the code is wrong?

Identifier does support null strings. Why did you choose to use the empty string instead of the null string? Are you aware that there's a difference between the empty string and the null string?

[Allan Sandfeld Jensen]

(In reply to comment #8)
> > Because the code is wrong?
> 
> Identifier does support null strings. Why did you choose to use the empty string instead of the null string? Are you aware that there's a difference between the empty string and the null string?

Calling Identifier(GlobalData*, const char*) will because there is no such direct constructor end up calling Identifier(GlobalData*, String&) which calls Identifier(GlobalData*, StringImpl*) which calls Identifier::addSlowCase(GlobalData, StringImpl* r) which contains the assertion    ASSERT(r->length()); and finally crashing when the null-string is added to the identifier table.

I returned the empty string identifier because that is what is generated for both null and empty strings input by all of the constructor except the default constructor and the StringImpl* constructor which asserts against it. 

Since the case I was running into was an empty string turning into a null-string identifier. It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers. But converting an empty-string to an invalid null string that is asserted against and is not good, even if it has been broken for 4 years.

[Geoffrey Garen]

> It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers.

Indeed. Why did you decide against that?

[Allan Sandfeld Jensen]

(In reply to comment #10)
> > It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers.
> 
> Indeed. Why did you decide against that?

Because the other constructors calls the one of add() functions which return an empty-string identifier for all strings of length 0. Since only the default constructor returned a null string identifier it lookslike a pattern where the null-string identifer is only meant to represent an uninitialized identifier, which makes sense if identifiers are mainly used as keys in tables where you need to be able to recognize defined from undefined entries. Under such a pattern a function returning an identifier should always return a defined identifer, so non-null. 

Of course if I am wrong about the pattern for Identifiers I expected developers who know better to correct me.

[Allan Sandfeld Jensen]

Created attachment 174383 [details]
Patch

Rename bug and split empty and null string handling

[EFL EWS Bot]

Comment on attachment 174383 [details]
Patch

Attachment 174383 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14845403

[Early Warning System Bot]

Comment on attachment 174383 [details]
Patch

Attachment 174383 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14826987

[Early Warning System Bot]

Comment on attachment 174383 [details]
Patch

Attachment 174383 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14857158

[Allan Sandfeld Jensen]

Created attachment 174387 [details]
Patch

[Jocelyn Turcotte]

Blocking the release since this crashes the fancybrowser example.

[WebKit Review Bot]

Comment on attachment 174387 [details]
Patch

Clearing flags on attachment: 174387

Committed r136246: <http://trac.webkit.org/changeset/136246>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.