Bug 40097 - Chromium NotificationPresenter tries to ref objects that are being destroyed
Summary: Chromium NotificationPresenter tries to ref objects that are being destroyed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: John Gregg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-02 18:57 PDT by Peter Kasting
Modified: 2010-06-17 02:29 PDT (History)
2 users (show)

See Also:


Attachments
Emergency fix as landed (1.57 KB, patch)
2010-06-02 19:00 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2010-06-03 14:21 PDT, John Gregg
no flags Details | Formatted Diff | Diff
Patch (4.76 KB, patch)
2010-06-03 14:53 PDT, John Gregg
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2010-06-14 12:09 PDT, John Gregg
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2010-06-14 15:14 PDT, John Gregg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2010-06-02 18:57:02 PDT
After r60569 to fix bug 39998, Chromium code began consistently failing an ASSERT in RefCounted.  This is because NotificationPresenterImpl::notificationObjectDestroyed(), which is called with a Notification object that is being destroyed, tries to ref the object.  This is clearly wrong, but the code presumably used to not ASSERT due to other refcounting problems fixed by the aforementioned checkin.

I'm going to make an emergency fix to simply do nothing in this function, since a large number of different Chromium tests are failing at the moment.  The right fix is presumably to move where this is called to be before the Notification is actually deleted, or perhaps to avoid refing the object here (although that seems perilous too).
Comment 1 Peter Kasting 2010-06-02 19:00:55 PDT
Created attachment 57725 [details]
Emergency fix as landed

Here's the fix I landed in r60590.
Comment 2 Yael 2010-06-02 19:20:13 PDT
Sorry for causing this trouble.
Please let me know if you are making changes to the Notifications design, because I am in the process of upstreaming the Qt implementation for notifications. thanks!
Comment 3 John Gregg 2010-06-03 14:21:58 PDT
Created attachment 57814 [details]
Patch
Comment 4 John Gregg 2010-06-03 14:53:12 PDT
Created attachment 57816 [details]
Patch
Comment 5 Yael 2010-06-03 16:44:02 PDT
With the current code in WebKit, a notification object can outlive the page that created it. So you can navigate to another page, and the notification is still visible.
This change is going to caouse the notification object to be destroyed when navigating to another page. Is that change intentional?
Comment 6 John Gregg 2010-06-03 16:52:51 PDT
I don't see how the patch necessarily causes any change to the lifetime of the object. It depends on what the presenter does in notificationObjectDestroyed(), though. 

For example, if that function is {}, I think this patch is a no-op (outside of the chromium part)
Comment 7 John Gregg 2010-06-03 16:54:36 PDT
Perhaps notificationObjectDestroyed is a poor name for that function anyway.  The point is to inform the presenter not to call events on the Notification anymore even if it chooses to continue showing it.  

Maybe notificationDetached (or just contextDestroyed) would be a better name.
Comment 8 Yael 2010-06-03 17:03:53 PDT
(In reply to comment #6)
> I don't see how the patch necessarily causes any change to the lifetime of the object. It depends on what the presenter does in notificationObjectDestroyed(), though. 
> 
> For example, if that function is {}, I think this patch is a no-op (outside of the chromium part)

Thanks for the explanation.
I guess the difference in our approaches is that in Chrome you can continue to show a notification after the JS object was destroyed, but in my code, I did not allow for that. I'll have to see how to adjust.
Comment 9 David Levin 2010-06-14 11:35:52 PDT
Comment on attachment 57816 [details]
Patch

> Index: WebCore/ChangeLog

> +
> +        No new tests. (OOPS!)

Needs explanation.
Comment 10 John Gregg 2010-06-14 12:09:51 PDT
Created attachment 58683 [details]
Patch
Comment 11 Yael 2010-06-14 14:02:31 PDT
Comment on attachment 58683 [details]
Patch

I have a couple of comments, even though I am not a reviewer :-)
>  
> +void Notification::contextDestroyed()
> +{
> +    if (m_presenter)
> +        m_presenter->notificationObjectDestroyed(this);
> +}
>  

This should call the base class to set m_scriptExecutionObject=0.
It is possible for the Notification object to be deleted before contextDestroyed() is called, and in those cases you will end up not calling notificationObjectDestroyed() at all.
Comment 12 John Gregg 2010-06-14 15:12:44 PDT
(In reply to comment #11)
> (From update of attachment 58683 [details])
> I have a couple of comments, even though I am not a reviewer :-)
> >  
> > +void Notification::contextDestroyed()
> > +{
> > +    if (m_presenter)
> > +        m_presenter->notificationObjectDestroyed(this);
> > +}
> >  
> 
> This should call the base class to set m_scriptExecutionObject=0.

Done.

> It is possible for the Notification object to be deleted before contextDestroyed() is called, and in those cases you will end up not calling notificationObjectDestroyed() at all.

It's only necessary to call notificationObjectDestroyed() after calling show().  Are you talking about cases where the object is actually used in a script context, then deleted before contextDestroyed() is called, or other cases where it's deleted before being attached to a context?
Comment 13 John Gregg 2010-06-14 15:14:31 PDT
Created attachment 58708 [details]
Patch
Comment 14 WebKit Commit Bot 2010-06-17 02:29:33 PDT
Comment on attachment 58708 [details]
Patch

Clearing flags on attachment: 58708

Committed r61316: <http://trac.webkit.org/changeset/61316>
Comment 15 WebKit Commit Bot 2010-06-17 02:29:38 PDT
All reviewed patches have been landed.  Closing bug.