Bug 40097 – Chromium NotificationPresenter tries to ref objects that are being destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Peter Kasting]

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).

[Peter Kasting]

Created attachment 57725 [details]
Emergency fix as landed

Here's the fix I landed in r60590.

[Yael]

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!

[John Gregg]

Created attachment 57814 [details]
Patch

[John Gregg]

Created attachment 57816 [details]
Patch

[Yael]

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?

[John Gregg]

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)

[John Gregg]

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.

[Yael]

(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.

[David Levin]

Comment on attachment 57816 [details]
Patch

> Index: WebCore/ChangeLog

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

Needs explanation.

[John Gregg]

Created attachment 58683 [details]
Patch

[Yael]

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.

[John Gregg]

(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?

[John Gregg]

Created attachment 58708 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 58708 [details]
Patch

Clearing flags on attachment: 58708

Committed r61316: <http://trac.webkit.org/changeset/61316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.