Bug 29835 – Clarify usage of functions related to sudden termination

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Evan Stade]

Created attachment 40272 [details]
sudden

bug found by Darin Fisher.

[Darin Adler]

Comment on attachment 40272 [details]
sudden

This change isn't needed on the normal Mac OS X version of WebKit. That's because you can make multiple calls to disableSuddenTermination and enableSuddenTermination, and only when you have balanced each disableSuddenTermination call with an enableSuddenTermination call will it do anything. I don't see any reason to merge together the before-unload set's disableSuddenTermination call with the plain-old-unload set's disableSuddenTermination.

Maybe there's a bug here, but this big a change is not needed to fix it.

[Evan Stade]

I see. So the bug is only in chromium then, because it takes {enable,disable}SuddenTermination to mean what the name says, that is, enable enables and does not just add to a counter. Mac and Chromium are the only two platforms that actually use this right now, but this mistake seems likely to crop up again as other platforms implement it. Perhaps documentation ought to be added to SuddenTermination.h.

[Darin Adler]

OK. SuddenTermination.h is designed to be a light wrapper around Mac OS X’s sudden termination API. The documentation for that is here:

http://developer.apple.com/mac/library/documentation/Cocoa/Reference/Foundation/Classes/NSProcessInfo_Class/Reference/Reference.html

If you'd like to add comments to SuddenTermination.h to make that more clear, I'd be happy to review the patch.

[Evan Stade]

Created attachment 40275 [details]
comment

instead just add comments to SuddenTermination.h

[Darin Adler]

Comment on attachment 40275 [details]
comment

The name is Mac OS X, not MacOSX.

I think the description makes things a bit more obscure than necessary. The idea is that you call disableSuddenTermination first, then balance it with a call to enableSuddenTermination. It's a programming error to call enableSuddenTermination more times than you called disableSuddenTermination.

But it's good to have a comment.

Comment seems OK to land as-is, but it would be even better to fix either of these things mentioned above.

[Eric Seidel (no email)]

Comment on attachment 40275 [details]
comment

Given that Darin suggested additional changes, marking this cq-.  If you'd like to make some of those changes you can re-post this with r?/cq? for automatic commit.  Otherwise someone can commit this manually and make the desired comment tweeks.

[Evan Stade]

sorry I have been slow at fixing this up. I will get around to it today.

[Evan Stade]

Created attachment 41468 [details]
updated comment

[Evan Stade]

Created attachment 41469 [details]
updated comment (correct .diff)

uploaded wrong patch

[Adam Barth]

Committed r49836: <http://trac.webkit.org/changeset/49836>