Re: Whitelists non-std %IteratorPrototype%.next (issue 222570043 by erights@gmail.com)

2 views
Skip to first unread message

kpr...@google.com

unread,
Apr 20, 2015, 1:39:11 PM4/20/15
to eri...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js#newcode3693
src/com/google/caja/ses/repairES5.js:3693: ses.optForeignForIn =
inTestFrame(function(window) {
Without having any specific ideas in mind: note that inTestFrame wasn't
written to be defensive — I wrote it solely for doing cross-frame repair
tests, i.e. without more widely exposing its results. Might be good to
review inTestFrame with that in mind.

Also, please mark this as private in some way, such as a leading _
(precedent with ses._EarlyStringMap above). We need to work on defining
our public interface more precisely, so that future changes can be
evaluated for whether they can break clients, and this is a small first
step.

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js#newcode3715
src/com/google/caja/ses/repairES5.js:3715: // error. No reliable brand
test for error anyway.
"brand test for Error"

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/startSES.js#newcode266
src/com/google/caja/ses/startSES.js:266: // Note: Imperative update, but
should be ok.
The danger of an imperative update is not localized to the update.
Please document the fact that we're doing this at additional locations:
where whitelist is defined, and where it is next read — that is,
sufficient for a maintainer to tell that this code must not be
_reordered_.

https://codereview.appspot.com/222570043/

eri...@gmail.com

unread,
Apr 21, 2015, 7:00:44 AM4/21/15
to kpr...@google.com, kpreid.sw...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com
All your's.


https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js#newcode3693
src/com/google/caja/ses/repairES5.js:3693: ses.optForeignForIn =
inTestFrame(function(window) {
On 2015/04/20 17:39:10, kpreid_google wrote:
> Without having any specific ideas in mind: note that inTestFrame
wasn't written
> to be defensive — I wrote it solely for doing cross-frame repair
tests, i.e.
> without more widely exposing its results. Might be good to review
inTestFrame
> with that in mind.

Enhanced comments on itTestFrame.

> Also, please mark this as private in some way, such as a leading _
(precedent
> with ses._EarlyStringMap above). We need to work on defining our
public
> interface more precisely, so that future changes can be evaluated
for whether
> they can break clients, and this is a small first step.

Done

https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js#newcode3715
src/com/google/caja/ses/repairES5.js:3715: // error. No reliable brand
test for error anyway.
On 2015/04/20 17:39:10, kpreid_google wrote:
> "brand test for Error"

Done.

https://codereview.appspot.com/222570043/

kpr...@google.com

unread,
Apr 21, 2015, 1:15:57 PM4/21/15
to eri...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com
LGTM

As discussed privately, I will be submitting this on MarkM's behalf
because he's away and we want this fixed soon.


https://codereview.appspot.com/222570043/diff/100001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/222570043/diff/100001/src/com/google/caja/ses/repairES5.js#newcode1321
src/com/google/caja/ses/repairES5.js:1321: // the ones we cannot test?
FYI: The problem is we don't know what the structure of the caller's
document is. It is _normally_ at least
<html><head></head><body></body></html>, but we cannot guarantee that it
contains any of those nodes.

The first case is the most correct document structure.
The second case is less correct but preferable over the alternatives if
<body> is missing (e.g. if the document is still being parsed, or the
document is a frameset).
The third case is necessary if nothing but a documentElement (<html>)
exists.
The fourth case is necessary if a documentElement does not exist (a
highly unusual but valid state of the DOM).

https://codereview.appspot.com/222570043/

kpr...@google.com

unread,
Apr 21, 2015, 1:22:35 PM4/21/15
to eri...@gmail.com, google-ca...@googlegroups.com, re...@codereview-hr.appspotmail.com
On 2015/04/21 17:15:56, kpreid_google wrote:
> LGTM

> As discussed privately, I will be submitting this on MarkM's behalf
because he's
> away and we want this fixed soon.

Committed and pushed to master @
0f80d73437eb4a2b21cc0c905ee1d170126635ae.

https://codereview.appspot.com/222570043/
Reply all
Reply to author
Forward
0 new messages