Crash properly on iOS
Created attachment 201045 [details] Patch
Comment on attachment 201045 [details] Patch Attachment 201045 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/436022
Comment on attachment 201045 [details] Patch Attachment 201045 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/380470
Comment on attachment 201045 [details] Patch Attachment 201045 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/274187
Comment on attachment 201045 [details] Patch Attachment 201045 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/318308
Comment on attachment 201045 [details] Patch Attachment 201045 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/406485
Comment on attachment 201045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201045&action=review I'm concerned that this change will break aggregation with existing crash reports. Can you try a known crash with and without this change to compare? > Source/WTF/wtf/Assertions.cpp:343 > + ((void(*)())0)(); /* More reliable, but doesn't say BBADBEEF */ This comment now looks as if it claims that this is more reliable than __builtin_trap().
<rdar://problem/13842771>
(In reply to comment #7) > (From update of attachment 201045 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201045&action=review > > I'm concerned that this change will break aggregation with existing crash reports. Can you try a known crash with and without this change to compare? You have WTFCrash on top for crash reporter. Would that be a problem? WTFReportBacktrace already skip 2 frames so it is just one level deeper. > > Source/WTF/wtf/Assertions.cpp:343 > > + ((void(*)())0)(); /* More reliable, but doesn't say BBADBEEF */ > > This comment now looks as if it claims that this is more reliable than __builtin_trap(). To be honest I have no clue what this comment mean by "More reliable".
Created attachment 201117 [details] Patch
Comment on attachment 201117 [details] Patch Attachment 201117 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/375014
Comment on attachment 201117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201117&action=review This changes things so that all crashes happen inside a function. The old code went out of its way to crash at the call site, not inside a function. I think the reason we wanted that was to make debugging easier. Not sure if that is important any more. Seems a little risky to change, though. I’m assuming the Windows build failures are due to changes in the exported symbols that are needed. > Source/WTF/wtf/Assertions.cpp:333 > +static void invokeCrashHook() > { > if (globalHook) > globalHook(); > } Why is this still a function? Why not just call the hook directly in WTFCrash? > Source/WTF/wtf/Assertions.cpp:339 > + (*(int *)(uintptr_t)0xbbadbeef = 0); There are extra unneeded parentheses here. > Source/WTF/wtf/Assertions.cpp:344 > + (*(int *)(uintptr_t)0xbbadbeef = 0); > +#if COMPILER(CLANG) > + __builtin_trap(); > +#else > + ((void(*)())0)(); > +#endif Really could use a why comment explaining the bbadbeef thing.
(In reply to comment #9) > > > Source/WTF/wtf/Assertions.cpp:343 > > > + ((void(*)())0)(); /* More reliable, but doesn't say BBADBEEF */ > > > > This comment now looks as if it claims that this is more reliable than __builtin_trap(). > > To be honest I have no clue what this comment mean by "More reliable". More reliable meant that in some cases accessing 0xbbadbeef would not cause a crash. That could be valid memory that happens to be mapped in, particularly on a 64-bit system. Whereas calling 0 or __builtin_trap was guaranteed to actually cause a crash.
Created attachment 201451 [details] Patch
Comment on attachment 201451 [details] Patch Clearing flags on attachment: 201451 Committed r150196: <http://trac.webkit.org/changeset/150196>
All reviewed patches have been landed. Closing bug.