[Webkit-unassigned] [Bug 52819] Crash in WebCore::HistoryController::itemsAreClones

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52819


Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81018|review?                     |review-
               Flag|                            |




--- Comment #20 from Brady Eidson <beidson at apple.com>  2011-02-03 08:52:25 PST ---
(From update of attachment 81018)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.