| Summary: | Autogenerated IDBFactory.open() does the wrong thing if you pass an explicit 'undefined' as the second argument | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> |
| Component: | Bindings | Assignee: | Chris Dumez <cdumez> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | alecflett, buildbot, cdumez, cgarcia, commit-queue, darin, esprehn+autocc, jsbell, kondapallykalyan, rniwa, sam, youennf |
| Priority: | P2 | ||
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=156946 | ||
| Bug Depends on: | 156955 | ||
| Bug Blocks: | 149117 | ||
| Attachments: | |||
This was discovered with the w3c platform test found here: http://w3c-test.org/IndexedDB/idbfactory_open9.htm I can look into this soon if this helps 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. Retitling for clarity: Autogenerated IDBFactory.open() does the wrong thing if you pass an explicit 'undefined' as the second argument (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. 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. Created attachment 277135 [details]
WIP patch
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. 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. 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 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
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 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
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 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
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 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
(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. Created attachment 277194 [details]
Patch
With this patch, all tests at http://w3c-test.org/IndexedDB/idbfactory_open9.htm were now passing. 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. 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 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
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 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
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 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
Comment on attachment 277194 [details]
Patch
Will investigate the failures.
Created attachment 277199 [details]
Patch
Created attachment 277202 [details]
Patch
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. Created attachment 277206 [details]
Patch
Comment on attachment 277206 [details] Patch Clearing flags on attachment: 277206 Committed r199970: <http://trac.webkit.org/changeset/199970> All reviewed patches have been landed. Closing bug. 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. (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. (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. 👍👍👍 |
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