Bug 31683 - JSC bindings for HasIndexGetter generates incorrect code (affects MediaList and CSSStyleDeclaration)
: JSC bindings for HasIndexGetter generates incorrect code (affects MediaList a...
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 20709
  Show dependency treegraph
 
Reported: 2009-11-19 13:51 PST by
Modified: 2009-11-24 09:05 PST (History)


Attachments
Patch (14.13 KB, patch)
2009-11-20 15:34 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Fixed ChangeLog (13.19 KB, patch)
2009-11-20 15:36 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Removed DOMTokenList part from CodeGeneratorJS.pm (13.52 KB, patch)
2009-11-20 16:58 PST, Erik Arvidsson
no flags Review Patch | Details | Formatted Diff | Diff
Reverted ws changes to CSSVariablesDeclaration.idl (12.77 KB, patch)
2009-11-20 17:36 PST, Erik Arvidsson
eric: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-19 13:51:20 PST
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.
------- Comment #1 From 2009-11-19 21:42:27 PST -------
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.
------- Comment #2 From 2009-11-20 15:34:06 PST -------
Created an attachment (id=43620) [details]
Patch
------- Comment #3 From 2009-11-20 15:36:31 PST -------
Created an attachment (id=43621) [details]
Fixed ChangeLog
------- Comment #4 From 2009-11-20 15:40:05 PST -------
CodeGeneratorJS.pm has a remnant from another git branch. I'll clean that up.
------- Comment #5 From 2009-11-20 15:43:08 PST -------
(From update of attachment 43621 [details])
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.
------- Comment #6 From 2009-11-20 15:55:29 PST -------
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.
------- Comment #7 From 2009-11-20 16:58:06 PST -------
Created an attachment (id=43630) [details]
Removed DOMTokenList part from CodeGeneratorJS.pm
------- Comment #8 From 2009-11-20 17:36:35 PST -------
Created an attachment (id=43634) [details]
Reverted ws changes to CSSVariablesDeclaration.idl
------- Comment #9 From 2009-11-21 07:15:43 PST -------
(From update of attachment 43634 [details])
This looks right to me, but since Darin reviewed the previous patch, we should wait just a bit in case he has further comments.
------- Comment #10 From 2009-11-21 12:12:23 PST -------
(From update of attachment 43634 [details])
OK
------- Comment #11 From 2009-11-23 21:18:22 PST -------
(From update of attachment 43634 [details])
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
------- Comment #12 From 2009-11-24 09:00:11 PST -------
I committed this manually.
------- Comment #13 From 2009-11-24 09:01:00 PST -------
http://trac.webkit.org/changeset/51326
------- Comment #14 From 2009-11-24 09:03:21 PST -------
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?
------- Comment #15 From 2009-11-24 09:05:33 PST -------
(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.