Bug 109245 - [GTK] TestRunner needs to implement originsWithApplicationCache
Summary: [GTK] TestRunner needs to implement originsWithApplicationCache
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-07 17:25 PST by Kwang Yul Seo
Modified: 2017-03-11 10:48 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.29 KB, patch)
2013-02-07 17:32 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2013-02-07 17:35 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2013-02-07 19:02 PST, Kwang Yul Seo
pnormand: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-02-07 17:25:53 PST
TestRunner needs to implement originsWithApplicationCache().

This is needed by the following test case:
http/tests/appcache/origins-with-appcache.html
Comment 1 Kwang Yul Seo 2013-02-07 17:32:43 PST
Created attachment 187208 [details]
Patch
Comment 2 Kwang Yul Seo 2013-02-07 17:35:42 PST
Created attachment 187209 [details]
Patch
Comment 3 Martin Robinson 2013-02-07 18:20:42 PST
Comment on attachment 187209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187209&action=review

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:868
> +JSValueRef DumpRenderTreeSupportGtk::originsWithApplicationCache(JSContextRef context)
> +{
> +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash> origins;
> +    cacheStorage().getOriginsWithCache(origins);
> +    Vector<JSValueRef> jsOriginsVector;
> +
> +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>::iterator it = origins.begin();
> +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>::iterator end = origins.end();
> +    for ( ; it != end; ++it) {
> +        String origin = (*it)->databaseIdentifier();
> +        JSRetainPtr<JSStringRef> originJS(Adopt, JSStringCreateWithCharacters(origin.characters(), origin.length()));
> +        jsOriginsVector.append(JSValueMakeString(context, originJS.get()));
> +    }
> +
> +    return JSObjectMakeArray(context, jsOriginsVector.size(), jsOriginsVector.data(), 0);
> +}

It'd probably make sense for this to return Vector<String> and then to convert that to an array in DumpRenderTree.
Comment 4 Kwang Yul Seo 2013-02-07 18:29:07 PST
(In reply to comment #3)
> It'd probably make sense for this to return Vector<String> and then to convert that to an array in DumpRenderTree.

Okay. It sounds better. I will upload a new patch.
Comment 5 Kwang Yul Seo 2013-02-07 19:02:09 PST
Created attachment 187217 [details]
Patch
Comment 6 Martin Robinson 2013-02-07 21:15:55 PST
Comment on attachment 187217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187217&action=review

> Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:660
> +        JSRetainPtr<JSStringRef> originJS(Adopt, JSStringCreateWithCharacters(origin.characters(), origin.length()));
> +        jsOriginsVector.append(JSValueMakeString(context, originJS.get()));

Hrm. Upon closer examination of this code I think there's an issue here. You are creating RetainPtr that is going out of scope immediately. I suspect that the memory you are using is either no longer allocated or susceptible to garbage collection.
Comment 7 Kwang Yul Seo 2013-02-08 01:01:00 PST
(In reply to comment #6)
> > Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:660
> > +        JSRetainPtr<JSStringRef> originJS(Adopt, JSStringCreateWithCharacters(origin.characters(), origin.length()));
> > +        jsOriginsVector.append(JSValueMakeString(context, originJS.get()));
> 
> Hrm. Upon closer examination of this code I think there's an issue here. You are creating RetainPtr that is going out of scope immediately. I suspect that the memory you are using is either no longer allocated or susceptible to garbage collection.

I think there is no problem because the newly created JSValue created by JSValueMakeString retains the given string and releases it upon garbage collection.

Please refer to the documentation of JSValueMakeString:

http://developer.apple.com/library/mac/#documentation/JavaScriptCore/Reference/JSValueRef_header_reference/Reference/reference.html#//apple_ref/doc/uid/TP40011501
Comment 8 Martin Robinson 2013-02-08 07:57:01 PST
(In reply to comment #7)

> I think there is no problem because the newly created JSValue created by JSValueMakeString retains the given string and releases it upon garbage collection.

This is only safe if JSValueMakeString cannot itself cause garbage collection. Another thing to keep in mind is that if you are relying on this property, you don't need a JSRetainPtr. I don't know now whether JSValueMakeString can cause a garbage collection, but if it can there's some issues here.
Comment 9 Kwang Yul Seo 2013-02-11 21:16:30 PST
(In reply to comment #8)
> This is only safe if JSValueMakeString cannot itself cause garbage collection. Another thing to keep in mind is that if you are relying on this property, you don't need a JSRetainPtr. I don't know now whether JSValueMakeString can cause a garbage collection, but if it can there's some issues here.

I see your point. I will check it again when I get back to work from the holiday.
Comment 10 Philippe Normand 2013-04-12 12:36:32 PDT
Comment on attachment 187217 [details]
Patch

Interesting patch but the issue Martin spotted seems quite serious. I suspect the patch also needs a rebase :/