Bug 34663

Summary:

Avoid crash due to Xcode 3.1 linker (radar 7070016)

Product:

WebKit

Reporter:

Stephen White <senorblanco>

Component:

New Bugs

Assignee:

Stephen White <senorblanco>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, dglazkov, eric, fishd, mark, mitz, pkasting, simon.fraser

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.5

Attachments:

Description	Flags
Stack trace from test_shell crash	none
Possible fix (NULL ptr check)	none
Patch	none
Patch	abarth: review+
Patch	dglazkov: review+

Stephen White

Reported 2010-02-05 14:18:50 PST

[Chromium] Crash in Chrome/Mac Release test_shell

Attachments
Stack trace from test_shell crash (3.10 KB, text/plain) 2010-02-05 14:22 PST, Stephen White	no flags	Details
Possible fix (NULL ptr check) (1.31 KB, patch) 2010-02-05 14:30 PST, Stephen White	no flags	Details Formatted Diff Diff
Patch (2.09 KB, patch) 2010-02-06 10:34 PST, Dimitri Glazkov (Google)	no flags	Details Formatted Diff Diff
Patch (2.36 KB, patch) 2010-02-06 10:40 PST, Dimitri Glazkov (Google)	abarth: review+	Details Formatted Diff Diff
Patch (2.64 KB, patch) 2010-02-08 08:19 PST, Stephen White	dglazkov: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Stephen White

Comment 1 2010-02-05 14:20:39 PST

This can be reproduced by building the Release test_shell, and invoking it without any arguments. Stack trace is attached.

Stephen White

Comment 2 2010-02-05 14:22:54 PST

Created attachment 48261 [details] Stack trace from test_shell crash

Stephen White

Comment 3 2010-02-05 14:30:35 PST

Created attachment 48262 [details] Possible fix (NULL ptr check)

Eric Seidel (no email)

Comment 4 2010-02-05 14:58:17 PST

Comment on attachment 48262 [details] Possible fix (NULL ptr check) I don't understand why deviceRGBColorSpaceRef() would ever fail.

Eric Seidel (no email)

Comment 5 2010-02-05 14:58:48 PST

I don't think deviceRGBColorSpaceRef is meant to be null checked. When is the system not going to be able to give you a color space for the device?

Stephen White

Comment 6 2010-02-05 15:02:54 PST

(In reply to comment #5) > I don't think deviceRGBColorSpaceRef is meant to be null checked. When is the > system not going to be able to give you a color space for the device? Well, the chromium canaries have been doing it for a day now, which is blocking me from rolling DEPS in Chrome: http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20Mac%20(webkit.org)/builds/14302 (At least, I *think* this is the same crash. Since they don't have symbols, it's tough to tell). I do think there is a deeper problem here, but I'm not enough of a Macspert to figure it out.

Dimitri Glazkov (Google)

Comment 7 2010-02-06 10:34:00 PST

Created attachment 48288 [details] Patch

Dimitri Glazkov (Google)

Comment 8 2010-02-06 10:40:14 PST

Created attachment 48289 [details] Patch

Adam Barth

Comment 9 2010-02-06 10:43:08 PST

Comment on attachment 48289 [details] Patch ok. was there a perf reason for having this inline?

Adam Barth

Comment 10 2010-02-06 10:44:26 PST

Also, should we be using DEFINE_STATIC_LOCAL, or whatever that nutty macro is called?

Dimitri Glazkov (Google)

Comment 11 2010-02-06 11:04:04 PST

Landed as http://trac.webkit.org/changeset/54465. We should also ensure that this pattern doesn't repeat elsewhere.

Peter Kasting

Comment 12 2010-02-06 11:27:25 PST

(In reply to comment #11) > We should also ensure that this pattern doesn't repeat elsewhere. What about the function directly below the one you patched? It uses the same pattern. (The FIXME comments no longer seem applicable either maybe?)

Dimitri Glazkov (Google)

Comment 13 2010-02-06 11:37:51 PST

(In reply to comment #10) > Also, should we be using DEFINE_STATIC_LOCAL, or whatever that nutty macro is > called? No, because CGColorSpaceRef is a pointer.

Peter Kasting

Comment 14 2010-02-06 12:21:50 PST

Reopening due to at least one other function (just below the first one in the same file) using this pattern. Turns out this is tripping an Xcode linker bug in the 3.1 toolchain, radar 7070016, which is fixed in 3.2. The patch method here avoids triggering the bug. Thanks to Mark Mentovai for tracking this down.

Darin Adler

Comment 15 2010-02-07 13:08:24 PST

(In reply to comment #14) > Turns out this is tripping an Xcode linker bug in the 3.1 toolchain, radar > 7070016, which is fixed in 3.2. The patch method here avoids triggering the > bug. And we need to support Xcode 3.1?

Stephen White

Comment 16 2010-02-08 07:27:35 PST

(In reply to comment #15) > (In reply to comment #14) > > Turns out this is tripping an Xcode linker bug in the 3.1 toolchain, radar > > 7070016, which is fixed in 3.2. The patch method here avoids triggering the > > bug. > > And we need to support Xcode 3.1? I'm afraid so, for the time being. The chromium buildbots and canaries all use Xcode 3.1, and there are some blocking issues that prevent us switching to Xcode 3.2 just yet.

Stephen White

Comment 17 2010-02-08 08:19:40 PST

Created attachment 48334 [details] Patch

Dimitri Glazkov (Google)

Comment 18 2010-02-08 09:13:13 PST

Comment on attachment 48334 [details] Patch thanks!

Stephen White

Comment 19 2010-02-08 12:35:22 PST

Committed r54502: <http://trac.webkit.org/changeset/54502>

Note You need to log in before you can comment on or make changes to this bug.