Bug 101867

Created attachment 173518 [details] Patch

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.

(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.

(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()".

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?

(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?

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.

> 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?

(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.

> 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?

(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.

Created attachment 174383 [details] Patch Rename bug and split empty and null string handling

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

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

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

Created attachment 174387 [details] Patch

Blocking the release since this crashes the fancybrowser example.

Comment on attachment 174387 [details] Patch Clearing flags on attachment: 174387 Committed r136246: <http://trac.webkit.org/changeset/136246>

All reviewed patches have been landed. Closing bug.