Bug 39998 – Notification object ref counting is not correct.

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

[Yael]

JSNotification objects are marked for garbage collection right after being created and are deleted shortly after, even if they have event listeners.

A patch is coming soon.

[Yael]

Created attachment 57551 [details]
Patch, fix the refcounting.

[John Gregg]

I'm not a reviewer, but looks good to me -- that's the same fix I've been testing locally.

[Darin Adler]

Comment on attachment 57551 [details]
Patch, fix the refcounting.

> -        Notification* createHTMLNotification(const String& URI, ExceptionCode& ec)
> +        PassRefPtr<Notification> createHTMLNotification(const String& URI, ExceptionCode& ec)

This change is great.

> -            return Notification::create(m_scriptExecutionContext->completeURL(URI), context(), ec, presenter());
> +            return adoptRef(Notification::create(m_scriptExecutionContext->completeURL(URI), context(), ec, presenter()));

This seems wrong. The adoptRef should be inside Notification::create and it should return a PassRefPtr!

[Yael]

Created attachment 57599 [details]
Move the adoptRef to Notification::create()

[WebKit Commit Bot]

Comment on attachment 57599 [details]
Move the adoptRef to Notification::create()

Clearing flags on attachment: 57599

Committed r60547: <http://trac.webkit.org/changeset/60547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60547 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60544
http://trac.webkit.org/changeset/60545
http://trac.webkit.org/changeset/60546
http://trac.webkit.org/changeset/60547
http://trac.webkit.org/changeset/60543

[Csaba Osztrogonác]

It was rolled out by http://trac.webkit.org/changeset/60554 ,
because it made fast/overflow/overflow-with-local-background-attachment.html crash.

I tried to reproduce the crash manually. If I ran only this test or all fast/overflow tests, the crash didn't occured, but with all fast tests.
Unfortunately I can't reproduce it in debug mode. :(

[Csaba Osztrogonác]

Reopened because the patch rolled-out.

[Yael]

(In reply to comment #9)
> Reopened because the patch rolled-out.

Looking at the build results from last night, that test started crashing in 60543,and my patch was 60547.

I am going to try the commit queue again.

[Csaba Osztrogonác]

(In reply to comment #10)
> Looking at the build results from last night, that test started crashing in 60543,and my patch was 60547.
No. Just check http://build.webkit.org/waterfall?show=Qt%20Linux%20Release
60543 - 60547 were on blame list, because 60542 was built in Build 12683 and 60547 was built in Build 12684. And then I tested it manually and 60547 was the culprit (Build 12690)

[Yael]

(In reply to comment #11)
> (In reply to comment #10)
> > Looking at the build results from last night, that test started crashing in 60543,and my patch was 60547.
> No. Just check http://build.webkit.org/waterfall?show=Qt%20Linux%20Release
> 60543 - 60547 were on blame list, because 60542 was built in Build 12683 and 60547 was built in Build 12684. And then I tested it manually and 60547 was the culprit (Build 12690)

Yes, you are right.
However, I just verified that if I apply 40003 and then 39998, the crash does not happen anymore.
I was afraid of temporary regression due to me splitting the notification work into smaller patches, and I guess this is what happened :-)

[Yael]

Recommitted as 60569.

After 60566, I could not reproduce the crash that caused this patch to be rolled out before.

[Simon Hausmann]

Revision r60547 cherry-picked into qtwebkit-2.1 with commit 9c33399bef9921ae60fe5d6ee573b9e7569eef84