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
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 20709
  Show dependency treegraph
 
Reported: 2009-11-19 13:51 PST by Erik Arvidsson
Modified: 2009-11-24 09:05 PST (History)
1 user (show)

See Also:


Attachments
Patch (14.13 KB, patch)
2009-11-20 15:34 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Fixed ChangeLog (13.19 KB, patch)
2009-11-20 15:36 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Removed DOMTokenList part from CodeGeneratorJS.pm (13.52 KB, patch)
2009-11-20 16:58 PST, Erik Arvidsson
no flags 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-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 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 Erik Arvidsson 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 Erik Arvidsson 2009-11-20 15:34:06 PST
Created attachment 43620 [details]
Patch
Comment 3 Erik Arvidsson 2009-11-20 15:36:31 PST
Created attachment 43621 [details]
Fixed ChangeLog
Comment 4 Erik Arvidsson 2009-11-20 15:40:05 PST
CodeGeneratorJS.pm has a remnant from another git branch. I'll clean that up.
Comment 5 Darin Adler 2009-11-20 15:43:08 PST
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.
Comment 6 Erik Arvidsson 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 Erik Arvidsson 2009-11-20 16:58:06 PST
Created attachment 43630 [details]
Removed DOMTokenList part from CodeGeneratorJS.pm
Comment 8 Erik Arvidsson 2009-11-20 17:36:35 PST
Created attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl
Comment 9 Eric Seidel 2009-11-21 07:15:43 PST
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.
Comment 10 Darin Adler 2009-11-21 12:12:23 PST
Comment on attachment 43634 [details]
Reverted ws changes to CSSVariablesDeclaration.idl

OK
Comment 11 WebKit Commit Bot 2009-11-23 21:18:22 PST
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
Comment 12 Erik Arvidsson 2009-11-24 09:00:11 PST
I committed this manually.
Comment 13 Erik Arvidsson 2009-11-24 09:01:00 PST
http://trac.webkit.org/changeset/51326
Comment 14 Erik Arvidsson 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 Erik Arvidsson 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.