Bug 84980 – Random crashes in webkit if a compiler addresses C++ defect #391

• Component: WebCore Misc.
• Status: RESOLVED INVALID

The compiler I have addresses C++ core language defect 391 mentioned at http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#391 

There is code in the webcore/dom/Element.cpp that is relying on the old behavior by keeping a reference to a temporary object (It works only if the reference was created by calling the copy ctor). If the compiler fixes the defect mentioned, following code becomes unsafe (results in random memory corruption).

line 661 - const AtomicString& localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;

After changing the code as follows, the problem goes away (Basically, removing the reference).

const AtomicString localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;

It is possible that there are other places in the code affected by the same bug.

Thanks.

[Alexey Proskuryakov]

I don't see how resolution of defect 391 makes this code ill-formed (not that I'm very much of a C++ language expert). Still, can it be that your compiler just has another unrelated bug?

We certainly have code like this in a number of places, not necessarily always with ?: operator.

(In reply to comment #1)
> I don't see how resolution of defect 391 makes this code ill-formed (not that I'm very much of a C++ language expert). Still, can it be that your compiler just has another unrelated bug?
> 
> We certainly have code like this in a number of places, not necessarily always with ?: operator.

My reading of the defect is that earlier, the compiler could copy construct a reference from a temporary rvalue object. Now, it may be possible that instead of calling copy ctor, it just keeps a reference to it directly. Since the temporary goes away after the line/statement completion, the object reference is invalid?

[Alexey Proskuryakov]

I'll ask around, but my understanding is that the temporary does not go away when bound to a reference like this.

The reason to use this idiom is to avoid copying. How I read defect 391 is that resolving it guarantees that copy constructor won't be invoked (basically forcing compilers to make "const AtomicString& localName" more efficient than "AtomicString localName" in this example).

(In reply to comment #3)
> I'll ask around, but my understanding is that the temporary does not go away when bound to a reference like this.
> 
> The reason to use this idiom is to avoid copying. How I read defect 391 is that resolving it guarantees that copy constructor won't be invoked (basically forcing compilers to make "const AtomicString& localName" more efficient than "AtomicString localName" in this example).

I think you might be right. I looked at generated code and it is possible that the new version of the compiler I have shipped with a bug while trying to fix other defect.

I'll post more info when I have it available.

Thanks.

[Anders Carlsson]

I asked Richard Smith who works on clang, and here's what he had to say:

<zygoloid> andersca: ugh. ok. in c++11, that code is fine, because the conditional operator will copy-initialize a temporary to hold its result, and the reference will extend the lifetime of that temporary.
<andersca> zygoloid: i see
<zygoloid> in c++03, the result is 'the value of the second or third expression'. we apply an lvalue-to-rvalue conversion on the third expression, but don't build a temporary. so it's not clear whether the result of the ?: is a temporary or not
<zygoloid> if it's not, 391 removes the permission for a temporary to be created, so the lifetime can't be extended
<andersca> ah!
<zygoloid> however... issue 446 fixes ?: to create a temporary whenever it produces a class prvalue
<zygoloid> imo a compiler which implements 391 but not 446 is broken

[Alexey Proskuryakov]

Ah, so this is how addressing defect 391 breaks it!

The compiler vendor was able to resolve the issue. Marking it invalid.

Thanks for the help.