Bug 41823

Summary: Fix adoptRef assertion failures caused by stack-allocated ResourceHandle objects
Product: WebKit Reporter: Darin Adler <darin>
Component: New BugsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, gustavo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 27672    
Attachments:
Description Flags
Patch
none
resourcehandle.diff none

Description Darin Adler 2010-07-07 18:10:51 PDT
Fix adoptRef assertion failures caused by stack-allocated ResourceHandle objects
Comment 1 Darin Adler 2010-07-07 18:12:31 PDT
Created attachment 60818 [details]
Patch
Comment 2 Darin Adler 2010-07-07 18:18:13 PDT
Comment on attachment 60818 [details]
Patch

Anders reviewed this.
Comment 3 Darin Adler 2010-07-07 18:19:44 PDT
Committed r62736: <http://trac.webkit.org/changeset/62736>
Comment 4 WebKit Review Bot 2010-07-07 18:39:10 PDT
http://trac.webkit.org/changeset/62736 might have broken GTK Linux 32-bit Release
Comment 5 Gustavo Noronha (kov) 2010-07-08 10:05:17 PDT
The failure reported by the bot seems to be indeed caused by this change. These two tests started timing out with the change, and reverting the change locally make them work again:

http/tests/xmlhttprequest/simple-cross-origin-denied-events-post-sync.html
http/tests/xmlhttprequest/simple-cross-origin-denied-events-sync.html
Comment 6 Darin Adler 2010-07-08 10:43:27 PDT
Uh oh. Any ideas for fixing that?
Comment 7 Xan Lopez 2010-07-08 11:09:44 PDT
(In reply to comment #6)
> Uh oh. Any ideas for fixing that?

Aren't you just never calling .start() in the handle after the patch?
Comment 8 Darin Adler 2010-07-08 11:10:41 PDT
(In reply to comment #7)
> Aren't you just never calling .start() in the handle after the patch?

I believe the synchronous loading function now calls create(), which calls start(). So that should work.
Comment 9 Xan Lopez 2010-07-08 11:18:03 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Aren't you just never calling .start() in the handle after the patch?
> 
> I believe the synchronous loading function now calls create(), which calls start(). So that should work.

I see, you are right. I guess the explanation is that it's failing in the previous checks in the create function now, otherwise the code seems identical.
Comment 10 Darin Adler 2010-07-08 11:20:01 PDT
(In reply to comment #9)
> I see, you are right. I guess the explanation is that it's failing in the previous checks in the create function now, otherwise the code seems identical.

I guess instead of calling create we should just explicitly do adoptRef(new) and start() to keep things working for now, and consider adding a FIXME saying we should use create. Xan, do you want to use a new bug to track that change? Do you want me to do it, or would you do it?
Comment 11 Xan Lopez 2010-07-08 11:24:37 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > I see, you are right. I guess the explanation is that it's failing in the previous checks in the create function now, otherwise the code seems identical.
> 
> I guess instead of calling create we should just explicitly do adoptRef(new) and start() to keep things working for now, and consider adding a FIXME saying we should use create. Xan, do you want to use a new bug to track that change? Do you want me to do it, or would you do it?

Let me do the patch and test it here, I'll attach it in the bug and you can review it.
Comment 12 Xan Lopez 2010-07-08 12:28:16 PDT
Created attachment 60927 [details]
resourcehandle.diff

Sorry for the delay, a World recompile kicked in. This fixes things locally for me.
Comment 13 Darin Adler 2010-07-08 13:52:49 PDT
Comment on attachment 60927 [details]
resourcehandle.diff

WebCore/platform/network/soup/ResourceHandleSoup.cpp:665
 +      handle.get()->start(frame);
No need for get() here.
Comment 14 Xan Lopez 2010-07-08 14:02:09 PDT
Comment on attachment 60927 [details]
resourcehandle.diff

Landed in r62832 with the suggested fix.