Summary: | Notification object ref counting is not correct. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, hausmann, johnnyg, ossy, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 40051 | ||||||||
Bug Blocks: | 39995 | ||||||||
Attachments: |
|
Description
Yael
2010-06-01 07:53:55 PDT
Created attachment 57551 [details]
Patch, fix the refcounting.
I'm not a reviewer, but looks good to me -- that's the same fix I've been testing locally. 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! Created attachment 57599 [details]
Move the adoptRef to Notification::create()
Comment on attachment 57599 [details] Move the adoptRef to Notification::create() Clearing flags on attachment: 57599 Committed r60547: <http://trac.webkit.org/changeset/60547> All reviewed patches have been landed. Closing bug. 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 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. :( Reopened because the patch rolled-out. (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. (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) (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 :-) Recommitted as 60569. After 60566, I could not reproduce the crash that caused this patch to be rolled out before. Revision r60547 cherry-picked into qtwebkit-2.1 with commit 9c33399bef9921ae60fe5d6ee573b9e7569eef84 |