Bug 39998 - Notification object ref counting is not correct.
Summary: Notification object ref counting is not correct.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 40051
Blocks: 39995
  Show dependency treegraph
 
Reported: 2010-06-01 07:53 PDT by Yael
Modified: 2010-07-30 04:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch, fix the refcounting. (2.74 KB, patch)
2010-06-01 09:19 PDT, Yael
kenneth: review+
Details | Formatted Diff | Diff
Move the adoptRef to Notification::create() (3.47 KB, patch)
2010-06-01 15:02 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-06-01 07:53:55 PDT
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.
Comment 1 Yael 2010-06-01 09:19:31 PDT
Created attachment 57551 [details]
Patch, fix the refcounting.
Comment 2 John Gregg 2010-06-01 09:22:42 PDT
I'm not a reviewer, but looks good to me -- that's the same fix I've been testing locally.
Comment 3 Darin Adler 2010-06-01 13:33:00 PDT
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!
Comment 4 Yael 2010-06-01 15:02:57 PDT
Created attachment 57599 [details]
Move the adoptRef to Notification::create()
Comment 5 WebKit Commit Bot 2010-06-02 01:45:56 PDT
Comment on attachment 57599 [details]
Move the adoptRef to Notification::create()

Clearing flags on attachment: 57599

Committed r60547: <http://trac.webkit.org/changeset/60547>
Comment 6 WebKit Commit Bot 2010-06-02 01:46:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Csaba Osztrogonác 2010-06-02 03:36:58 PDT
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. :(
Comment 9 Csaba Osztrogonác 2010-06-02 05:06:35 PDT
Reopened because the patch rolled-out.
Comment 10 Yael 2010-06-02 05:29:50 PDT
(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.
Comment 11 Csaba Osztrogonác 2010-06-02 05:34:44 PDT
(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)
Comment 12 Yael 2010-06-02 06:33:54 PDT
(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 :-)
Comment 13 Yael 2010-06-02 10:17:33 PDT
Recommitted as 60569.

After 60566, I could not reproduce the crash that caused this patch to be rolled out before.
Comment 14 Simon Hausmann 2010-07-30 04:12:38 PDT
Revision r60547 cherry-picked into qtwebkit-2.1 with commit 9c33399bef9921ae60fe5d6ee573b9e7569eef84