event handler in <script...for> should not get executed

[Johnny(Jianning) Ding]

According to bug 21193 and 16915, the event handler in <script...for> should not get executed. The CL (http://trac.webkit.org/changeset/43061) was to fix this problem.
However, in current implementation, if a script tag has type or language attribute, the event handler in <script...for> can be executed in WebKit.

If you copy the following example code in a html page, a alert box will be seen.
<script type="text/javascript" for="Object1" event="onclick"> alert("click pic"); </script>

I write a patch to fix it. Will upload soon.

[Johnny(Jianning) Ding]

Created attachment 43328 [details]
patch v1 to fix this issue

[Darin Adler]

Comment on attachment 43328 [details]
patch v1 to fix this issue

> +    // No type or language is specified, so we assume the script to be JavaScript.
> +    bool isJavaScript = true;

I don't think it makes sense to move this comment up to the top of the function without changing it. Perhaps if you are moving it, it could say "// Assume the script is JavaScript if no type or language is specified."

>      String type = m_scriptElement->typeAttributeValue();
>      if (!type.isEmpty())
> -        return MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower());
> +        isJavaScript = MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower());
>  
>      String language = m_scriptElement->languageAttributeValue();
>      if (!language.isEmpty())
> -        return isSupportedJavaScriptLanguage(language);
> +        isJavaScript = isSupportedJavaScriptLanguage(language);
>  
> -    // No type or language is specified, so we assume the script to be JavaScript.

I think it would be good to use nested ifs and return early if the answer is known to be false. You would not need a boolean then. If you really did want the boolean, then maybe we could factor that check out into a separate function for clarity. Here is my suggested nested if form:

    String type = m_scriptElement->typeAttributeValue();
    if (!type.isEmpty()) {
        if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()))
            return false;
    } else {
        String language = m_scriptElement->languageAttributeValue();
        if (!language.isEmpty() && !isSupportedJavaScriptLanguage(language))
            return false;
    }

Your change to the code changes the logic, causing the language to take precedence over the type if both are present. The old code would check the type first and return a result based only on the type. The new code checks the language second, and overwrites the value based on the type. This should cause at least one existing test case to fail. If it doesn't, then we need better test cases!

> -    String forAttribute = m_scriptElement->forAttributeValue();
> -    return forAttribute.isEmpty();
> +    if (isJavaScript) {
> +        String forAttribute = m_scriptElement->forAttributeValue();
> +        return forAttribute.isEmpty();
> +    }
> +    return false;

We normally would do an early exit here.

    if (!isJavaScript)
        return false;

And leave the other code in place. If you take my suggestion about changing things around then you won't need to change this part of the function at all, though.

review- because this reverses the precedence between type and language by accident.

[Johnny(Jianning) Ding]

Thanks Darin!
You are right, my previous patch did change the check logic of type and
language. (I added "if (!isJavaScript)" check before checking language on my
chromium build, but somehow I missed it when writing patch on Mac WebKit
build.)

According to your comments, I dropped the isJavaScript variable.
Please take another look!

[Johnny(Jianning) Ding]

Created attachment 43333 [details]
patch v2 for fix

[Sam Weinig]

*** Bug 16915 has been marked as a duplicate of this bug. ***

[Johnny(Jianning) Ding]

HI Darin

Will you or other committers help on landing this patch, right? I am still working on earning the committers privilege:)

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/51097