[webkit-reviews] review denied: [Bug 52819] Crash in WebCore::HistoryController::itemsAreClones : [Attachment 81018] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Charles Reis <creis at chromium.org>'s
request for review:
Bug 52819: Crash in WebCore::HistoryController::itemsAreClones
https://bugs.webkit.org/show_bug.cgi?id=52819

Attachment 81018: Patch
https://bugs.webkit.org/attachment.cgi?id=81018&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81018&action=review

I understand the goal is to keep certain items Ref'ed, as previously they were
being destroyed before last-use.  However this is mis-using (Pass)RefPtrs.

PassRefPtr is for transferring ownership, which you're not actually doing
(except for the one case I pointed out).

If that one case is the only one necessary to change, then that's all you
should change.

If the other changes are necessary, you need to leave them as raw pointers and
simply RefPtr<> them within the function body where you think they might
unintentionally be destroyed.

> Source/WebCore/loader/HistoryController.cpp:613
>  {

With item, you are potentially passing ownership, and it makes sense to use a
PassRefPtr.  For fromItem, I only see a .get() comparison, so the PassRefPtr
isn't needed.

> Source/WebCore/loader/HistoryController.cpp:621
>  

m_provisionalItem is already a RefPtr - Why do you need the extra targetItem
RefPtr here?  You're adding ref-count churn which is surprisingly bad for
performance.

> Source/WebCore/loader/HistoryController.cpp:640
>  {

Same thing here - PassRefPtrs are for actually passing ownership, and I don't
see that you are passing ownership in this method.