Bug 47055 - All the WebSocket tests crash
Summary: All the WebSocket tests crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-03 01:03 PDT by Adam Barth
Modified: 2010-10-03 11:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2010-10-03 01:05 PDT, Adam Barth
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-10-03 01:03:50 PDT
All the WebSocket tests crash
Comment 1 Adam Barth 2010-10-03 01:05:48 PDT
Created attachment 69583 [details]
Patch
Comment 2 Holger Freyther 2010-10-03 01:13:32 PDT
Comment on attachment 69583 [details]
Patch

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

You could keep the 'const'. The patch looks sane.

> WebCore/ChangeLog:9
> +        That doesn't work in C++.  Instead, we need to actually store the

You could remove the extra space.
Comment 3 Adam Barth 2010-10-03 01:19:50 PDT
Committed r68984: <http://trac.webkit.org/changeset/68984>
Comment 4 Darin Adler 2010-10-03 10:37:36 PDT
The C++ standard, section 12.5, paragraph 5, says:

"The temporary to which the reference is bound [...] persists for the lifetime of the reference except as specified below.”

And the text below does not list any exception which applies here.

So the temporary objects here will last the lifetime of the reference. So this code change should not have had any effect at all.
Comment 5 Alexey Proskuryakov 2010-10-03 11:41:43 PDT
This is a fairly common idiom in WebCore code. People have submitted patches to "fix" such code several times in the past, but reviewers stopped that before, and the problem always turned out to be something different.

When did this start? Did this happen on buildbots?
Comment 6 Holger Freyther 2010-10-03 11:47:42 PDT
(In reply to comment #5)
> This is a fairly common idiom in WebCore code. People have submitted patches to "fix" such code several times in the past, but reviewers stopped that before, and the problem always turned out to be something different.

I was not aware that it is valid until the end of the block. I was doing this example http://paste.lisp.org/display/115156 for adam to show that it is at least valid for function calls.

The other thing is, I think it is to be preferred to not use const reference as it can have suprising effects and as long as the function being called is inline, gcc will be able to eliminate the copy (since early gcc 3.x), as can be seen when compiling/executing the example.

Anyway, I will be more careful.


> 
> When did this start? Did this happen on buildbots?

Leaving that to Adam.