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

[bugzilla-daemon at webkit.org]

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





--- Comment #21 from Brady Eidson <beidson at apple.com>  2011-02-03 08:53:27 PST ---
(In reply to comment #20)
> (From update of attachment 81018 [details])
> 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.

Sorry for the commenting fail here - the inline review comments feature never does what I intend.  sigh.  Hopefully lining up the line numbers manually will make it obvious what I meant.

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