| Summary: | JSC bindings generator should generate deletable JSC functions | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Zan Dobersek <zan> |
| Component: | New Bugs | Assignee: | Zan Dobersek <zan> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | buildbot, cdumez, commit-queue, eric.carlson, esprehn+autocc, ggaren, glenn, jer.noble, kangil.han, kondapallykalyan, rniwa |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 124151 | ||
| Bug Blocks: | 122213 | ||
| Attachments: | |||
|
Description
Zan Dobersek
2013-10-06 12:54:46 PDT
Created attachment 213522 [details]
Patch
Comment on attachment 213522 [details]
Patch
Does this change the behavior of any existing LayoutTests?
Can you add a LayoutTest for a representative set of bindings? The bindings unit tests are nice, but they don't verify end-to-end behavior.
No change occurs in the Web-facing interfaces. All the WebIDL operations remain non-configurable due to the OperationsNotDeletable attribute with which I annotated all the interfaces that declare operations. All the generated JSC functions remain non-deletable, I checked that by inspecting the generated bindings before and after this change. The plan is to now go through the affected interfaces and check conformity with other browsers and also the relevant specifications, removing the OperationsNotDeletable attribute to achieve the configurability. Interfaces used by the Web Inspector and the testing internals are affected, so those operations become configurable as soon as this lands. I can produce a test based on some operation under windows.internal. Comment on attachment 213522 [details]
Patch
I don’t think we should land this with the only testing being the binding tests. I’d like to have at least one operation that is deletable, and I’d like to see a regression test demonstrating the behavior on an actual DOM function, whether it's 100% correct or not.
Otherwise, the first time we will expose this behavior to webpages will be on the first operation added after this change to a class that does not have OperationsNotDeletable on it. And it seems unlikely the person doing that will realize they need to test this delete behavior.
Created attachment 213584 [details]
Patch
Comment on attachment 213584 [details]
Patch
I’m mixed up about this. The test only tests that getOwnPropertyDescriptor claims these functions are writable, enumerable, and configurable. I was hoping for a test that enumerates, writes, and configures instead of one that takes getOwnPropertyDescriptor's word for it.
But maybe I am misunderstanding the issue. What’s the real web compatibility risk here? If there is none, then it seems strange that we stuck OperationsNotDeletable in all those files! What testing should we do when we go to remove them?
(In reply to comment #6) > (From update of attachment 213584 [details]) > I’m mixed up about this. The test only tests that getOwnPropertyDescriptor claims these functions are writable, enumerable, and configurable. I was hoping for a test that enumerates, writes, and configures instead of one that takes getOwnPropertyDescriptor's word for it. Hmm, understood. > But maybe I am misunderstanding the issue. What’s the real web compatibility risk here? If there is none, then it seems strange that we stuck OperationsNotDeletable in all those files! What testing should we do when we go to remove them? I'd say the risk is small, but not zero. I proposed a step-by-step change in behavior to have the maximum control of what's affected in a single patch and that the introduced changes match other browsers in behavior. The approach might be an overkill. We're trying to match a specification that is already in the Candidate Recommendation phase[1]. Both IE (tested version 11) and Firefox (tested versions 24 and 27) declare the WebIDL operations configurable, while Chrome (tested version 31) doesn't. But I don't have enough experience in making changes in Web-facing APIs to feel comfortable determining whether the risk is small enough to bypass the taxing cross-browser testing. Apart from testing cross-browser compatibility, additional tests could be added that would use the idlharness.js[2] script to test conformance to the WebIDL spec by parsing the specified IDL and testing its expected behavior on a given object. The object here would have to be something that's generated by the JSC bindings generator, so either something that's part of the Web-facing API or is put under window.internals for testing purposes only. [1] http://www.w3.org/TR/WebIDL/ (Note that the Unforgeable attribute that would make operations non-configurable is being specified in the second edition of WebIDL.) [2] https://github.com/w3c/testharness.js/blob/master/idlharness.js Comment on attachment 213584 [details]
Patch
Looks like this patch needs two things:
(1) The regression test should test overwriting, deleting, and enumerating DOM properties, in addition to testing the return value of getOwnPropertyDescriptor.
(2) The patch should make an opinionated decision about whether IDL functions should be writeable, deletable, and/or enumerable. It seems like your opinion is "yes", in which case, you should remove OperationsNotDeletable from IDL.
(3) Do any of these need to be marked [Unforgeable]? If so, the patch needs to do that too, or it's a security regression.
There's no perfect way to assess the risk of (2), so I think we just need to try it, and see what happens.
(In reply to comment #8) > (From update of attachment 213584 [details]) > Looks like this patch needs two things: > > (1) The regression test should test overwriting, deleting, and enumerating DOM properties, in addition to testing the return value of getOwnPropertyDescriptor. OK, will add. > (2) The patch should make an opinionated decision about whether IDL functions should be writeable, deletable, and/or enumerable. It seems like your opinion is "yes", in which case, you should remove OperationsNotDeletable from IDL. > ... > There's no perfect way to assess the risk of (2), so I think we just need to try it, and see what happens. The attribute was added to most of the interfaces, with the idea to at first preserve non-configurability and then remove the attribute after checking the cross-browser compatibility. I can shuffle back on that idea (and remove the attributes before the generator changes) if the risk is not that high and we're ready to address any backfiring -- it would definitely save me some time. FWIW, Firefox and IE already have configurable operations, and we're chasing the spec here, if that in any way reduces the risk. > (3) Do any of these need to be marked [Unforgeable]? If so, the patch needs to do that too, or it's a security regression. The bindings generator doesn't yet support the Unforgeable attribute, but in the latest drafts the only unforgeable operations seem to be defined under the Location interface[1]. The Location interface could keep the OperationsNotDeletable attribute to enforce non-configurability of these operations until support for the Unforgeable attribute is added. [1] http://www.w3.org/html/wg/drafts/html/master/single-page.html#location Created attachment 216428 [details]
Patch
Updated test case, with actual tests for writable, enumerable and configurable behavior.
> > There's no perfect way to assess the risk of (2), so I think we just need to try it, and see what happens. > > The attribute was added to most of the interfaces, with the idea to at first preserve non-configurability and then remove the attribute after checking the cross-browser compatibility. I can shuffle back on that idea (and remove the attributes before the generator changes) if the risk is not that high and we're ready to address any backfiring -- it would definitely save me some time. The key here is to not to leave ourselves in a half-way state. Since this patch only changes Node, we need a plan to change other interfaces soon. > FWIW, Firefox and IE already have configurable operations, and we're chasing the spec here, if that in any way reduces the risk. It does, somewhat. > > (3) Do any of these need to be marked [Unforgeable]? If so, the patch needs to do that too, or it's a security regression. > > The bindings generator doesn't yet support the Unforgeable attribute, but in the latest drafts the only unforgeable operations seem to be defined under the Location interface[1]. The Location interface could keep the OperationsNotDeletable attribute to enforce non-configurability of these operations until support for the Unforgeable attribute is added. If we're using OperationsNotDeletable to mean "not writeable, enumerable, or deletable", should it be OperationsNotConfigurable instead? Comment on attachment 216428 [details]
Patch
r=me
Please follow up with the rest of the DOM interfaces.
(In reply to comment #11) > > > (3) Do any of these need to be marked [Unforgeable]? If so, the patch needs to do that too, or it's a security regression. > > > > The bindings generator doesn't yet support the Unforgeable attribute, but in the latest drafts the only unforgeable operations seem to be defined under the Location interface[1]. The Location interface could keep the OperationsNotDeletable attribute to enforce non-configurability of these operations until support for the Unforgeable attribute is added. > > If we're using OperationsNotDeletable to mean "not writeable, enumerable, or deletable", should it be OperationsNotConfigurable instead? Using OperationsNotDeletable means that operations are writable and enumerable (which is not changing) but not deletable (which will change soon). 'Deletable' in JavaScriptCore is synonymous to 'Configurable' in ECMAScript. I don't have any preference on what the attribute is called -- after all, it will be removed at the very end. If nobody is opposed to it, I'll do the cross-browser compatibility testing with the patch only applied locally (i.e. not landed in trunk). That way the OperationsNotDeletable attribute removals would land first, with the generator change coming into full effect later. The Location interface should still keep the attribute, until there's support for the Unforgeable attribute. This basically nullifies the point of all the patches that added the attribute to a large share of interfaces, so special apologies to Darin, who reviewed most of those. Created attachment 216594 [details]
Patch for landing
Running the patch through the EWS.
Comment on attachment 216594 [details] Patch for landing Attachment 216594 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22599457 New failing tests: js/dom/getOwnPropertyDescriptor.html canvas/philip/tests/type.prototype.html compositing/tiling/tiled-in-iframe.html Created attachment 216609 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 216594 [details] Patch for landing Attachment 216594 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22938233 New failing tests: js/dom/getOwnPropertyDescriptor.html canvas/philip/tests/type.prototype.html js/basic-set.html compositing/tiling/tiled-in-iframe.html Created attachment 216610 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216594 [details] Patch for landing Attachment 216594 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22599466 New failing tests: js/dom/getOwnPropertyDescriptor.html canvas/philip/tests/type.prototype.html compositing/tiling/tiled-in-iframe.html Created attachment 216614 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 216640 [details]
Patch for landing
Again running the updated patch through EWS.
Committed r159100: <http://trac.webkit.org/changeset/159100> |