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).
Created attachment 57725 [details] Emergency fix as landed Here's the fix I landed in r60590.
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!
Created attachment 57814 [details] Patch
Created attachment 57816 [details] Patch
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?
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)
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.
(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 on attachment 57816 [details] Patch > Index: WebCore/ChangeLog > + > + No new tests. (OOPS!) Needs explanation.
Created attachment 58683 [details] Patch
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.
(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?
Created attachment 58708 [details] Patch
Comment on attachment 58708 [details] Patch Clearing flags on attachment: 58708 Committed r61316: <http://trac.webkit.org/changeset/61316>
All reviewed patches have been landed. Closing bug.