Bug 41823 – Fix adoptRef assertion failures caused by stack-allocated ResourceHandle objects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Darin Adler]

Fix adoptRef assertion failures caused by stack-allocated ResourceHandle objects

[Darin Adler]

Created attachment 60818 [details]
Patch

[Darin Adler]

Comment on attachment 60818 [details]
Patch

Anders reviewed this.

[Darin Adler]

Committed r62736: <http://trac.webkit.org/changeset/62736>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62736 might have broken GTK Linux 32-bit Release

[Gustavo Noronha (kov)]

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

[Darin Adler]

Uh oh. Any ideas for fixing that?

[Xan Lopez]

(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?

[Darin Adler]

(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.

[Xan Lopez]

(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.

[Darin Adler]

(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?

[Xan Lopez]

(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.

[Xan Lopez]

Created attachment 60927 [details]
resourcehandle.diff

Sorry for the delay, a World recompile kicked in. This fixes things locally for me.

[Darin Adler]

Comment on attachment 60927 [details]
resourcehandle.diff

WebCore/platform/network/soup/ResourceHandleSoup.cpp:665
 +      handle.get()->start(frame);
No need for get() here.

[Xan Lopez]

Comment on attachment 60927 [details]
resourcehandle.diff

Landed in r62832 with the suggested fix.