Bug 34663 - Avoid crash due to Xcode 3.1 linker (radar 7070016)
Summary: Avoid crash due to Xcode 3.1 linker (radar 7070016)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-05 14:18 PST by Stephen White
Modified: 2010-02-08 12:35 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2010-02-05 14:18:50 PST
[Chromium] Crash in Chrome/Mac Release test_shell
Comment 1 Stephen White 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.
Comment 2 Stephen White 2010-02-05 14:22:54 PST
Created attachment 48261 [details]
Stack trace from test_shell crash
Comment 3 Stephen White 2010-02-05 14:30:35 PST
Created attachment 48262 [details]
Possible fix (NULL ptr check)
Comment 4 Eric Seidel (no email) 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.
Comment 5 Eric Seidel (no email) 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?
Comment 6 Stephen White 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.
Comment 7 Dimitri Glazkov (Google) 2010-02-06 10:34:00 PST
Created attachment 48288 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2010-02-06 10:40:14 PST
Created attachment 48289 [details]
Patch
Comment 9 Adam Barth 2010-02-06 10:43:08 PST
Comment on attachment 48289 [details]
Patch

ok.  was there a perf reason for having this inline?
Comment 10 Adam Barth 2010-02-06 10:44:26 PST
Also, should we be using DEFINE_STATIC_LOCAL, or whatever that nutty macro is called?
Comment 11 Dimitri Glazkov (Google) 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.
Comment 12 Peter Kasting 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?)
Comment 13 Dimitri Glazkov (Google) 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.
Comment 14 Peter Kasting 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.
Comment 15 Darin Adler 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?
Comment 16 Stephen White 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.
Comment 17 Stephen White 2010-02-08 08:19:40 PST
Created attachment 48334 [details]
Patch
Comment 18 Dimitri Glazkov (Google) 2010-02-08 09:13:13 PST
Comment on attachment 48334 [details]
Patch

thanks!
Comment 19 Stephen White 2010-02-08 12:35:22 PST
Committed r54502: <http://trac.webkit.org/changeset/54502>