[webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more

[Maciej Stachowiak]

On Jun 18, 2011, at 10:47 PM, Alexey Proskuryakov wrote:

> 
> 18.06.2011, в 22:15, Maciej Stachowiak написал(а):
> 
>> - I think having a rule for using PassRefPtr for arguments that depends on how callers use the function is likely to be hard to use in practice, since it requires global knowledge of all callers to pick the right function signature. The rule I prefer is that if a function takes ownership of the argument, whether or not we currently have a caller that gives away ownership, the function should take PassRefPtr. If it does not take ownership, it should take a raw pointer. That way you can decide the right thing to do based solely on the definition of the function itself.
> 
> 
> Using PassRefPtr for arguments is a fairly common source of bugs. The way it's zeroed out after being used has caused crashes in code paths that weren't tested before commit for some reason (see e.g. <https://bugs.webkit.org/show_bug.cgi?id=52981>). Another example that we've just seen (<https://bugs.webkit.org/show_bug.cgi?id=62836>) was when a different compiler had a different order of evaluation, so building with it suddenly exposed a crash that we didn't need to have.
> 
> Looking at either bug, using PassRefPtr is pointless. For trace(), we could theoretically pass ownership from jsConsolePrototypeFunctionTrace(), but we do not, and micro-optimizing Console functions for performance isn't worth the cost of having had this crasher bug in my opinion. For bug 62836, it's quite clearly impossible to pass ownership to HTMLTableRowsCollection.

Yes, the self-zeroing can cause bugs. But passing raw pointers can also cause bugs, and possibly more serious ones because they'll be freed memory access instead of null pointer derefs. I can see three options:

(1) Use PassRefPtr for every parameter that takes ownership.

Pro:
    Bright-line rule; fairly clear how to apply it.
    Distributed performance benefit.

Con:
    Possible accidental self-zeroing bugs.
    Maybe slight code bloat if it leads to reefing at call sites.

(2) Abandon PassRefPtr for incoming function arguments entirely.

Pro:
    Extremely simple to apply the rule.
    No accidental-self-zeroing bugs.

Con:
     We probably lose some performance.
     Possible accidental freed memory access bugs.
     For cases where the object being passed really is freshly created, now you have to make a local RefPtr variable to hold it.

(3) Use PassRefPtr based on whether callers ever provide a PassRefPtr as an argument.

Pro:
     If done right, get the performance benefit without the costs/hazards.

Con:
     Applying this rule is hard; coding or reviewing a patch that changes PassRefPtrness of an argument in principle requires checking every call site.
     If people apply the rule wrong, we can get both null-deref and freed-memory bugs.

(3) is my least favorite.

Maybe there are other options or other pros and cons. It might be useful to make a wiki page if we want to collect more options and arguments.

Regards,
Maciej