Add a class for parsing application manifests

[David Quesada]

rdar://problem/34747949

[David Quesada]

Created attachment 324697 [details]
v1

[Joseph Pecoraro]

Comment on attachment 324697 [details]
v1

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

> Source/JavaScriptCore/ChangeLog:9
> +        * Configurations/FeatureDefines.xcconfig: Add ENABLE_APPLICATION_MANIFEST feature flag.

New features like ENABLE_APPLICATION_MANIFEST are typically announced on webkit-dev to let others know it is being worked on.

Likewise, you should probably update the "Web App Manifest" section of Source/WebCore/features.json.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> +        manifest = JSONParse(exec, String("{}"));

It seems a bit overkill to construct an empty object using JSONParse. Maybe this could just be `manifest = constructEmptyObject(exec);`.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> +        manifest = JSONParse(exec, String("{}"));

Ditto.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:126
> +        logDeveloperWarning("The \"start_url\" application manifest property must be same origin as the document.");

Nit: "must be same origin" => "must be the same origin"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:135
> +    return parseGenericString(manifest, "name");

Nit: To simplify `const char*` to `WTF::String` conversion here, you can wrap the literal like so:

    return parseGenericString(manifest, ASCIILiteral("name"));

This avoids a copy. You can do the same elsewhere in the patch, for example all calls to `logDeveloperWarning`.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:32
> +#include <wtf/Variant.h>

This include doesn't appear to be used.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:38
> +class VM;

This forward declaration doesn't appear to be used.

[David Quesada]

(In reply to Joseph Pecoraro from comment #2)
> Comment on attachment 324697 [details]
> v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324697&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        * Configurations/FeatureDefines.xcconfig: Add ENABLE_APPLICATION_MANIFEST feature flag.
> 
> New features like ENABLE_APPLICATION_MANIFEST are typically announced on
> webkit-dev to let others know it is being worked on.
> 
> Likewise, you should probably update the "Web App Manifest" section of
> Source/WebCore/features.json.

I'll do these.

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> > +        manifest = JSONParse(exec, String("{}"));
> 
> It seems a bit overkill to construct an empty object using JSONParse. Maybe
> this could just be `manifest = constructEmptyObject(exec);`.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> > +        manifest = JSONParse(exec, String("{}"));
> 
> Ditto.

Good idea. I didn't notice constructEmptyObject().

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:126
> > +        logDeveloperWarning("The \"start_url\" application manifest property must be same origin as the document.");
> 
> Nit: "must be same origin" => "must be the same origin"
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:135
> > +    return parseGenericString(manifest, "name");
> 
> Nit: To simplify `const char*` to `WTF::String` conversion here, you can
> wrap the literal like so:
> 
>     return parseGenericString(manifest, ASCIILiteral("name"));
> 
> This avoids a copy. You can do the same elsewhere in the patch, for example
> all calls to `logDeveloperWarning`.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:32
> > +#include <wtf/Variant.h>
> 
> This include doesn't appear to be used.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:38
> > +class VM;
> 
> This forward declaration doesn't appear to be used.

I'll fix these nits and remove the unused include/declaration. Thanks for the comments.

[David Quesada]

Created attachment 324726 [details]
Patch v2

Addressed JoePeck's comments - removed an unused import and declaration, used ASCIILiteral for string literals, updated web app manifest status in features.json.

[Brady Eidson]

Comment on attachment 324726 [details]
Patch v2

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

> Source/JavaScriptCore/runtime/StringPrototype.h:75
> +JS_EXPORT_PRIVATE JSValue jsTrimString(ExecState*, JSValue thisValue, int trimKind);

There's no need to expose this.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:61
> +    m_globalObject = JSC::JSGlobalObject::create(*vm, JSC::JSGlobalObject::createStructure(*vm, JSC::jsNull()));

We're creating a global object each time parseManifest is called? That's a bit of an extreme expense. Let's not recreate it each time.

Maybe parseManifest is only called once, so it's not like we're recreating m_globalObject? If so, you could just make it be a local global object.

An added optimization; Is this all main-thread only code? If so, all ApplicationManifestParser's could share one global object.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> +        logDeveloperWarning(ASCIILiteral("The application manifest must be valid JSON data."));

I think dev error warnings are usually descriptive, not prescriptive.

I'd suggest "The application manifest is not valid JSON data."

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> +        logDeveloperWarning(ASCIILiteral("The application manifest must be a JSON object."));

The error condition here isn't that it's not "a JSON object", but that the result is of JSON parsing is not even a Javascript object. (Is that possible?)

Also, describe the error, not the solution.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:91
> +    logDeveloperWarning("The \"" + propertyName + "\" application manifest property must be a string.");

"...is not a string"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:96
> +    logDeveloperWarning("The \"" + propertyName + "\" application manifest property must be a valid URL.");

"...is not a valid URL"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:126
> +    auto startURLOrigin = SecurityOrigin::create(startURL);
> +    auto documentOrigin = SecurityOrigin::create(documentURL);
> +
> +    if (!startURLOrigin->isSameOriginAs(documentOrigin.get())) {

Origins are pretty heavyweight objects and should not be used for string origin comparisons.

For that we have protocolHostAndPortAreEqual()

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:127
> +        logDeveloperWarning(ASCIILiteral("The \"start_url\" application manifest property must be the same origin as the document."));

A "URL" is not an "origin", therefore equating a URL value in the message to an origin is a bit nonsensical.

Perhaps: "The origin of the "start_url" property is not in the same origin as the document"

Additionally, you have both origins here. The message could read:
"The start_url's origin of "http://www.apple.com/" is different from the document's origin of "https://google.com""

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:167
> +    auto scopeOrigin = SecurityOrigin::create(scopeURL);
> +    auto targetOrigin = SecurityOrigin::create(targetURL);

See above about SecurityOrigins.

We can just do comparisons on the URLs.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:197
> +    auto scopeURLOrigin = SecurityOrigin::create(scopeURL);
> +    auto documentOrigin = SecurityOrigin::create(documentURL);
> +
> +    if (!scopeURLOrigin->isSameOriginAs(documentOrigin.get())) {

Ditto about origins.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +        logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest property must be same origin as the document."));

See above about formatting these messages.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:203
> +        logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest property must be within scope of the start URL."));

See above about formatting these messages.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:221
> +    return asString(JSC::jsTrimString(exec, value, JSC::TrimLeft | JSC::TrimRight))->tryGetValue();

There's no need to expose the JS internal trimString just to use it here.

Instead I would simply make the WTFString then call stripWhitespace() on it.

[David Quesada]

(In reply to Brady Eidson from comment #5)
> Comment on attachment 324726 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324726&action=review
> 
> > Source/JavaScriptCore/runtime/StringPrototype.h:75
> > +JS_EXPORT_PRIVATE JSValue jsTrimString(ExecState*, JSValue thisValue, int trimKind);
> 
> There's no need to expose this.

I originally chose to do this based on this wording from the spec: “When instructed to Trim(x), a user agent must behave as if [ECMASCRIPT]'s String.prototype.trim() function had been called on the string x.”

This actually matters in a few weird places where non-string could be passed to Trim(). But I’ll do this by executing the same steps without exposing the function from JavaScriptCore.

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:61
> > +    m_globalObject = JSC::JSGlobalObject::create(*vm, JSC::JSGlobalObject::createStructure(*vm, JSC::jsNull()));
> 
> We're creating a global object each time parseManifest is called? That's a
> bit of an extreme expense. Let's not recreate it each time.
> 
> Maybe parseManifest is only called once, so it's not like we're recreating
> m_globalObject? If so, you could just make it be a local global object.
> 
> An added optimization; Is this all main-thread only code? If so, all
> ApplicationManifestParser's could share one global object.

I can’t think of any reason this would be called off the main thread, so it seems reasonable to share a global object. I’ll do this.

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> > +        logDeveloperWarning(ASCIILiteral("The application manifest must be valid JSON data."));
> 
> I think dev error warnings are usually descriptive, not prescriptive.
> 
> I'd suggest "The application manifest is not valid JSON data."

I’ll change all the messages to be more descriptive.

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> > +        logDeveloperWarning(ASCIILiteral("The application manifest must be a JSON object."));
> 
> The error condition here isn't that it's not "a JSON object", but that the
> result is of JSON parsing is not even a Javascript object. (Is that
> possible?)

The spec says to log a warning “ If Type(manifest) is not "object": “. If the manifest data is a string, bool, or number, we would want to log a warning since we expect the top-level value to be an object.

> 
> Also, describe the error, not the solution.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:91
> > +    logDeveloperWarning("The \"" + propertyName + "\" application manifest property must be a string.");
> 
> "...is not a string"
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:96
> > +    logDeveloperWarning("The \"" + propertyName + "\" application manifest property must be a valid URL.");
> 
> "...is not a valid URL"
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:126
> > +    auto startURLOrigin = SecurityOrigin::create(startURL);
> > +    auto documentOrigin = SecurityOrigin::create(documentURL);
> > +
> > +    if (!startURLOrigin->isSameOriginAs(documentOrigin.get())) {
> 
> Origins are pretty heavyweight objects and should not be used for string
> origin comparisons.
> 
> For that we have protocolHostAndPortAreEqual()
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:127
> > +        logDeveloperWarning(ASCIILiteral("The \"start_url\" application manifest property must be the same origin as the document."));
> 
> A "URL" is not an "origin", therefore equating a URL value in the message to
> an origin is a bit nonsensical.
> 
> Perhaps: "The origin of the "start_url" property is not in the same origin
> as the document"
> 
> Additionally, you have both origins here. The message could read:
> "The start_url's origin of "http://www.apple.com/" is different from the
> document's origin of "https://google.com""
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:167
> > +    auto scopeOrigin = SecurityOrigin::create(scopeURL);
> > +    auto targetOrigin = SecurityOrigin::create(targetURL);
> 
> See above about SecurityOrigins.
> 
> We can just do comparisons on the URLs.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:197
> > +    auto scopeURLOrigin = SecurityOrigin::create(scopeURL);
> > +    auto documentOrigin = SecurityOrigin::create(documentURL);
> > +
> > +    if (!scopeURLOrigin->isSameOriginAs(documentOrigin.get())) {
> 
> Ditto about origins.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> > +        logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest property must be same origin as the document."));
> 
> See above about formatting these messages.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:203
> > +        logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest property must be within scope of the start URL."));
> 
> See above about formatting these messages.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:221
> > +    return asString(JSC::jsTrimString(exec, value, JSC::TrimLeft | JSC::TrimRight))->tryGetValue();
> 
> There's no need to expose the JS internal trimString just to use it here.
> 
> Instead I would simply make the WTFString then call stripWhitespace() on it.

[David Quesada]

Created attachment 324937 [details]
Patch v3

Addressed Brady's feedback and added ApplicationManifestParser.cpp to unified sources.

[David Quesada]

Created attachment 328171 [details]
Patch v4

Newest rebase, also started protecting ApplicationManifestParser's global JS object from GC, otherwise it can get deallocated after a while.

[Geoffrey Garen]

Comment on attachment 328171 [details]
Patch v4

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:41
> +static JSC::VM* globalVM()

It's overkill to dedicate a JavaScript VM to application manifest parsing -- unless we have a really good reason to do our parsing in parallel with other WebCore work.

You can use WebCore::commonVM() instead.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:52
> +    static JSC::JSGlobalObject* sharedGlobalObject;

Also feels like overkill to keep a global object around permanently just because you parsed a manifest once. That's a memory leak. I know that Brady asked for some caching, but I don't think the speed benefit justifies the permanent memory cost.

A better option, which resolves all concerns, is to use wtf/JSONValues.h to do the JSON parsing without a JavaScript environment. Let's make that change. It will simplify this code.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:118
> +void ApplicationManifestParser::logDeveloperWarning(const String& message)

Some of the messages you pass to this function don't have enough context to explain themselves. For example, "The start URL is not within scope of the provided scope URL" doesn't even say that you're parsing an application manifest. I think you want a helper function that prefixes everything with "Parsing application manifest <manifestURL>: <message>".

[David Quesada]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 328171 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328171&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:41
> > +static JSC::VM* globalVM()
> 
> It's overkill to dedicate a JavaScript VM to application manifest parsing --
> unless we have a really good reason to do our parsing in parallel with other
> WebCore work.
> 
> You can use WebCore::commonVM() instead.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:52
> > +    static JSC::JSGlobalObject* sharedGlobalObject;
> 
> Also feels like overkill to keep a global object around permanently just
> because you parsed a manifest once. That's a memory leak. I know that Brady
> asked for some caching, but I don't think the speed benefit justifies the
> permanent memory cost.
> 
> A better option, which resolves all concerns, is to use wtf/JSONValues.h to
> do the JSON parsing without a JavaScript environment. Let's make that
> change. It will simplify this code.

Will do.

> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:118
> > +void ApplicationManifestParser::logDeveloperWarning(const String& message)
> 
> Some of the messages you pass to this function don't have enough context to
> explain themselves. For example, "The start URL is not within scope of the
> provided scope URL" doesn't even say that you're parsing an application
> manifest. I think you want a helper function that prefixes everything with
> "Parsing application manifest <manifestURL>: <message>".

I'll change to use this wording.

[David Quesada]

Created attachment 328404 [details]
Patch v5

Use wtf/JSONValues.h instead of JavaScriptCore for parsing the manifest JSON.

[Geoffrey Garen]

Comment on attachment 328404 [details]
Patch v5

r=me

[WebKit Commit Bot]

Comment on attachment 328404 [details]
Patch v5

Clearing flags on attachment: 328404

Committed r225507: <https://trac.webkit.org/changeset/225507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35843475>