Bug 31683 – JSC bindings for HasIndexGetter generates incorrect code (affects MediaList and CSSStyleDeclaration)

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Erik Arvidsson]

When the IDL has an index getter that is also set to convert null strings we return undefined even though we should return null.

This happens with MediaList.idl and CSSStyleDeclaration.idl

The generated code looks like this:

    bool ok;
    unsigned index = propertyName.toUInt32(&ok, false);
    if (ok && index < static_cast<MediaList*>(impl())->length()) {
        slot.setCustomIndex(this, index, indexGetter);
        return true;
    }
    return getStaticValueSlot<JSMediaList, Base>(exec, &JSMediaListTable, this, propertyName, slot);

http://www.w3.org/TR/DOM-Level-2-Style/stylesheets.html#StyleSheets-MediaList says this about item

Returns the indexth in the list. If index is greater than or equal to the number of media in the list, this returns null.

One way to solve this is to always call the index getter if |ok| is true but that might expose bugs in the existing code. Another option is to not check the length if ConvertNullStringTo is present. That will solve my cases but it feels a bit hacky.

[Erik Arvidsson]

I got this working. I ended up checking whether IndexGetterReturnsStrings and in that case skip the length check. I also checked all the use cases of this (I hope) and only CSSVariableDeclaration needed some trivial changes.

I'll upload a patch tomorrow and also create a layout test for this.

[Erik Arvidsson]

Created attachment 43620 [details]
Patch

[Erik Arvidsson]

Created attachment 43621 [details]
Fixed ChangeLog

[Erik Arvidsson]

CodeGeneratorJS.pm has a remnant from another git branch. I'll clean that up.

[Darin Adler]

Comment on attachment 43621 [details]
Fixed ChangeLog

You say that these now return empty string according to "the spec". What spec? Does this also match other browsers? Which other browsers did you test.

Changing behavior could cause compatibility with sites that have worked with Safari, for example, for years, so we want to be sure this matches both the de facto standard and specifications.

> -    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or $type eq "CSSVariablesDeclaration" or $type eq "DOMTokenList";
> +    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or $type eq "CSSVariablesDeclaration";

This change seems separate from the others. Or is there some relationship? Do you have test cases for all the uses of DOMTokenList?

>      if (i >= length())
> -        return String();
> +        return "";

>      if (i >= m_properties.size())
> -       return String();
> +       return "";

>      if (index >= m_variableNames.size())
> -        return String();
> +        return "";

I only see two tests where the result is an empty string, so I'm not sure all three of these changes are covered by tests.

[Erik Arvidsson]

DOMTokenList is how I found this bug. That line of change should not be here. I screwed up with my git branch. I'm working on fixing that.

Here are the specs.

MediaList should return null
http://www.w3.org/TR/DOM-Level-2-Style/stylesheets.html#StyleSheets-MediaList

Firefox returns undefined here


CSSStyleDeclaration should return ""
http://www.w3.org/TR/DOM-Level-2-Style/css#CSS-CSSStyleDeclaration

Our implementations of this interface are CSSComputedStyleDeclaration, CSSMutableStyleDeclaration and these now return the empty string instead of null string.

Firefox returns "" for these


CSSVariablesDeclaration should return  ""
http://disruptive-innovations.com/zoo/cssvariables/#mozTocId496530

CSS variables never made it and are disabled. I can add a test but it will fail.


So there is a slight change compared to Firefox, the media list returns null instead of undefined. I think this should be safe.

[Erik Arvidsson]

Created attachment 43630 [details]
Removed DOMTokenList part from CodeGeneratorJS.pm

[Erik Arvidsson]

Created attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl

[Eric Seidel (no email)]

Comment on attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl

This looks right to me, but since Darin reviewed the previous patch, we should wait just a bit in case he has further comments.

[Darin Adler]

Comment on attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl

OK

[WebKit Commit Bot]

Comment on attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl

Rejecting patch 43634 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
Last 500 characters of output:
bCore/css/CSSComputedStyleDeclaration.cpp
Hunk #1 FAILED at 1478.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/css/CSSComputedStyleDeclaration.cpp.rej
patching file WebCore/css/CSSMutableStyleDeclaration.cpp
Hunk #1 FAILED at 636.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/css/CSSMutableStyleDeclaration.cpp.rej
patching file WebCore/css/CSSStyleDeclaration.idl
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/css/CSSStyleDeclaration.idl.rej

[Erik Arvidsson]

I committed this manually.

[Erik Arvidsson]

http://trac.webkit.org/changeset/51326

[Erik Arvidsson]

I see that the ChangeLogs got confused. I think git svn dcommit added the changelog or something like this. Should I go back and fix the changelogs?

[Erik Arvidsson]

(In reply to comment #14)
> I see that the ChangeLogs got confused. I think git svn dcommit added the
> changelog or something like this. Should I go back and fix the changelogs?

I'm confused. The diffs of the changelogs look correct but the message in trac duplicates the changelog message. It seems like it included both my git commit message and the changelog diff.