Bug 156939 – Autogenerated IDBFactory.open() does the wrong thing if you pass an explicit 'undefined' as the second argument

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Brady Eidson]

Autogenerated IDBFactory.open() does not allow `undefined` for the second argument

The IDL looks like this:
    [CallWith=ScriptExecutionContext, RaisesExceptionWithMessage] IDBOpenDBRequest open(DOMString name, [EnforceRange] optional unsigned long long version);

The generated code does the following:
1 - Checks to see if there's exactly 1 argument.
2 - If so, it calls IDBFactory::open(ScriptExecutionContext&, const String& name, ExceptionCodeWithMessage&);
3 - If there is more than one argument, it gets the second argument and converts it to a uint64_t with a range check.
4 - If the conversion failed, it throws an exception for the EnforceRange - "NaN is outside the range 0..2^64-1"

Instead, at step 3, it should allow an explicit "undefined" and call the first form of IDBFactory::open() from step #2

[Brady Eidson]

This was discovered with the w3c platform test found here:

http://w3c-test.org/IndexedDB/idbfactory_open9.htm

[Chris Dumez]

I can look into this soon if this helps

[Chris Dumez]

The generated bindings seem wrong here. They'll just convert undefined to 0. This will probably require tweaking the bindings generator and used Optional<> type for this parameter.

[Brady Eidson]

Retitling for clarity:
Autogenerated IDBFactory.open() does the wrong thing if you pass an explicit 'undefined' as the second argument

[Brady Eidson]

(In reply to comment #3)
> The generated bindings seem wrong here. They'll just convert undefined to 0.
> This will probably require tweaking the bindings generator and used
> Optional<> type for this parameter.

That's totally reasonable.

In fact, I *would* like to consolidate the multiple IDBFactory::open methods down to one. It seems silly that there's two just because of the optional argument.

If the bindings get changed to pass Optional<> for the second argument, IDBFactory can + should adopt that.

[Brady Eidson]

I think we've identified the path forward, but just to clarify an earlier point:

(In reply to comment #3)
> The generated bindings seem wrong here. They'll just convert undefined to 0.

Actually what they're doing is throwing a NaN exception because "undefined" is not a number in the expected range.

[Chris Dumez]

Created attachment 277135 [details]
WIP patch

[Chris Dumez]

Comment on attachment 277135 [details]
WIP patch

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

> Source/WebCore/bindings/scripts/IDLAttributes.txt:130
> +UseOptionalForOptionalNumbericParametersWithoutDefaultValue

The flag should be reversed. This should be the default behavior and we should have a flag such as DoNotUseWTFOptionalForParameters on interfaces that have not been ported yet.

Note that when porting interfaces, we don't necessarily have to switch the implementation to use Optional<>. In most cases (but not the IDBFactory.open case), we can simply specify a default value in the IDL:
e.g. 
[RaisesException] ScriptProcessorNode createScriptProcessor(unsigned long bufferSize, optional unsigned long numberOfInputChannels, optional unsigned long numberOfOutputChannels);

would become
[RaisesException] ScriptProcessorNode createScriptProcessor(unsigned long bufferSize, optional unsigned long numberOfInputChannels = 2, optional unsigned long numberOfOutputChannels = 2);

and then the C++ implementation would stay unchanged since we only use WTF::Optional for parameters without a default value.

[Brady Eidson]

Comment on attachment 277135 [details]
WIP patch

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

Thanks for tackling this!

Giving a review feedback comment to clarify where I thought this was heading:

> Source/WebCore/Modules/indexeddb/IDBFactory.h:57
>      RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, ExceptionCodeWithMessage&);
> -    RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, unsigned long long version, ExceptionCodeWithMessage&);
> +    RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, Optional<unsigned long long> version, ExceptionCodeWithMessage&);

The new method signature:
+ RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, Optional<unsigned long long> version, ExceptionCodeWithMessage&);
Seems great.

But my understanding was that the new mechanism would completely obsolete the version of open() without the version argument.

i.e., line 56 should be gone, and everything in the auto generated JSIDBFactoryOpenFunction would call the new line 57 version.

[Build Bot]

Comment on attachment 277135 [details]
WIP patch

Attachment 277135 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1205657

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open9.html
storage/indexeddb/intversion-bad-parameters-private.html
imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277137 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277135 [details]
WIP patch

Attachment 277135 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1205656

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open9.html
storage/indexeddb/intversion-bad-parameters-private.html
imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277138 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277135 [details]
WIP patch

Attachment 277135 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1205663

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open9.html
storage/indexeddb/intversion-bad-parameters-private.html
imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277139 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277135 [details]
WIP patch

Attachment 277135 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1205666

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open9.html
imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm

[Build Bot]

Created attachment 277140 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Chris Dumez]

(In reply to comment #9)
> Comment on attachment 277135 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277135&action=review
> 
> Thanks for tackling this!
> 
> Giving a review feedback comment to clarify where I thought this was heading:
> 
> > Source/WebCore/Modules/indexeddb/IDBFactory.h:57
> >      RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, ExceptionCodeWithMessage&);
> > -    RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, unsigned long long version, ExceptionCodeWithMessage&);
> > +    RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name, Optional<unsigned long long> version, ExceptionCodeWithMessage&);
> 
> The new method signature:
> + RefPtr<IDBOpenDBRequest> open(ScriptExecutionContext&, const String& name,
> Optional<unsigned long long> version, ExceptionCodeWithMessage&);
> Seems great.
> 
> But my understanding was that the new mechanism would completely obsolete
> the version of open() without the version argument.
> 
> i.e., line 56 should be gone, and everything in the auto generated
> JSIDBFactoryOpenFunction would call the new line 57 version.

Yes, I haven't had time to experiment with this part yet but I will.

[Chris Dumez]

Created attachment 277194 [details]
Patch

[Chris Dumez]

With this patch, all tests at http://w3c-test.org/IndexedDB/idbfactory_open9.htm were now passing.

[Chris Dumez]

Comment on attachment 277194 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3383
> +    return 0 if $type eq "boolean";

I will work on getting rid of these ASAP in follow-up patches. This is just so that it can be done incrementally.

[Build Bot]

Comment on attachment 277194 [details]
Patch

Attachment 277194 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1214187

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
storage/indexeddb/intversion-bad-parameters-private.html
media/media-source/media-source-end-of-stream-readyState.html
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277195 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277194 [details]
Patch

Attachment 277194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1214192

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
storage/indexeddb/intversion-bad-parameters-private.html
media/media-source/media-source-end-of-stream-readyState.html
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277196 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277194 [details]
Patch

Attachment 277194 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1214202

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
storage/indexeddb/intversion-bad-parameters-private.html
media/media-source/media-source-end-of-stream-readyState.html
storage/indexeddb/intversion-bad-parameters.html

[Build Bot]

Created attachment 277198 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Comment on attachment 277194 [details]
Patch

Will investigate the failures.

[Chris Dumez]

Created attachment 277199 [details]
Patch

[Chris Dumez]

Created attachment 277202 [details]
Patch

[Darin Adler]

Comment on attachment 277202 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3377
> +    # FIXME: Progressively extend this too all parameter types.

"to all parameter types"

I don’t the comment is clear enough. The idea is that we want to remove all these exceptions from the list one by one and then remove the function entirely. But I don’t think the comment makes that clear. This also seems like something that might belong in CodeGenerator.pm rather than in the JS-specific one, even if we are only implementing in JavaScript for now.

I am doing a similar project for enumerations, where future enumeration types will all use "enum class" in the C++ DOM but some are still using String for legacy reasons. In that case, I chose to put the function in CodeGenerator.pm and used a hash for the "black list".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3627
> +                    my $defaultValue = "Optional<" . GetNativeTypeFromSignature($parameter) . ">(Nullopt)";

How is Optional<X>(Nullopt) different from Optional<X>()? I don’t think it is.

[Chris Dumez]

Created attachment 277206 [details]
Patch

[Chris Dumez]

Comment on attachment 277206 [details]
Patch

Clearing flags on attachment: 277206

Committed r199970: <http://trac.webkit.org/changeset/199970>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 277206 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm:4
> -<script src="../../../resources/testharness.js"></script>
> -<script src="../../../resources/testharnessreport.js"></script>
> -<script src=support.js></script>
> +<script src=/resources/testharness.js></script>
> +<script src=/resources/testharnessreport.js></script>

Just noticed this. I thought we were trying to avoid merging this kind of change, because we wanted the tests to work if you open the file in the web browser with file URL scheme.

[Chris Dumez]

(In reply to comment #35)
> Comment on attachment 277206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277206&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm:4
> > -<script src="../../../resources/testharness.js"></script>
> > -<script src="../../../resources/testharnessreport.js"></script>
> > -<script src=support.js></script>
> > +<script src=/resources/testharness.js></script>
> > +<script src=/resources/testharnessreport.js></script>
> 
> Just noticed this. I thought we were trying to avoid merging this kind of
> change, because we wanted the tests to work if you open the file in the web
> browser with file URL scheme.

Oh, OK. I wasn't aware. I'll revert that part shortly then.

[Chris Dumez]

(In reply to comment #36)
> (In reply to comment #35)
> > Comment on attachment 277206 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=277206&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idbfactory_open9.htm:4
> > > -<script src="../../../resources/testharness.js"></script>
> > > -<script src="../../../resources/testharnessreport.js"></script>
> > > -<script src=support.js></script>
> > > +<script src=/resources/testharness.js></script>
> > > +<script src=/resources/testharnessreport.js></script>
> > 
> > Just noticed this. I thought we were trying to avoid merging this kind of
> > change, because we wanted the tests to work if you open the file in the web
> > browser with file URL scheme.
> 
> Oh, OK. I wasn't aware. I'll revert that part shortly then.

Done in r199973.

[Brady Eidson]

👍👍👍