Bug 115782 – Crash properly on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Crash properly on iOS

[Benjamin Poulain]

Created attachment 201045 [details]
Patch

[EFL EWS Bot]

Comment on attachment 201045 [details]
Patch

Attachment 201045 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/436022

[EFL EWS Bot]

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

[Early Warning System Bot]

Comment on attachment 201045 [details]
Patch

Attachment 201045 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/274187

[Early Warning System Bot]

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

[Build Bot]

Comment on attachment 201045 [details]
Patch

Attachment 201045 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/406485

[Alexey Proskuryakov]

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().

[Benjamin Poulain]

<rdar://problem/13842771>

[Benjamin Poulain]

(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".

[Benjamin Poulain]

Created attachment 201117 [details]
Patch

[Build Bot]

Comment on attachment 201117 [details]
Patch

Attachment 201117 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/375014

[Darin Adler]

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.

[Darin Adler]

(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.

[Benjamin Poulain]

Created attachment 201451 [details]
Patch

[Benjamin Poulain]

Comment on attachment 201451 [details]
Patch

Clearing flags on attachment: 201451

Committed r150196: <http://trac.webkit.org/changeset/150196>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.