Scanner: Fix bug and add coverage for iterators/generators. (issue 297960043 by kpreid@google.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: felix8a,

Description:
* Fix bad function call in Context. This bug was not found before
because it only shows up when trying to report an error on this
type of scan context.
* Coverage/fixes for iterator.next() and *IteratorPrototype.
* Coverage/fixes for generators.
* Add missing coverage for DOMSettableTokenList.

Please review this at https://codereview.appspot.com/297960043/

Affected files (+42, -5 lines):
M tests/com/google/caja/plugin/test-scan-core.js
M tests/com/google/caja/plugin/test-scan-guest.js


Index: tests/com/google/caja/plugin/test-scan-core.js
diff --git a/tests/com/google/caja/plugin/test-scan-core.js
b/tests/com/google/caja/plugin/test-scan-core.js
index
f4f3e3bea53a4213e39a9bc677f08adaba161906..468959e764aa84f1466a76e3e5152e7417bc8e9c
100644
--- a/tests/com/google/caja/plugin/test-scan-core.js
+++ b/tests/com/google/caja/plugin/test-scan-core.js
@@ -492,7 +492,9 @@ var scanning; // exports
// When invoking methods on a prototype, use an instance of
this
// ctor instead as 'this'.
var protoctx = makeContext(pval, subpath, depth + 1,
- function() {
+ function getSelfCOfPrototypeObject() {
+ // We have SomeCtor.prototype, and want to invoke a
method on
+ // it, so obtain an instance of it.
var selfC = getSelfC();
return makeContext(
obtainInstance(selfC.get(), selfC),
@@ -500,13 +502,12 @@ var scanning; // exports
depth + 1,
'self',
function() { return noThisContext; },
- protoctx,
function() {
return 'obtainInstance(' + selfC.getProgram()
+ ')';
});
},
getSelfC,
- function() {
+ function getProgramInfoOfPrototypeObject() {
return {
thisArg: pval,
src: context.getProgram() + '.prototype'
Index: tests/com/google/caja/plugin/test-scan-guest.js
diff --git a/tests/com/google/caja/plugin/test-scan-guest.js
b/tests/com/google/caja/plugin/test-scan-guest.js
index
a28eb2e237e3cc2a254289142ecd4b88f7a90f33..e89e68ea06a78f1cafdcaea57b9e33e0901cef0c
100644
--- a/tests/com/google/caja/plugin/test-scan-guest.js
+++ b/tests/com/google/caja/plugin/test-scan-guest.js
@@ -117,6 +117,10 @@
"unescape",
"URIError",
"WeakMap",
+
+ // our own specials
+ "ArrayIteratorPrototype_ScanPseudoCtor",
+ "StringIteratorPrototype_ScanPseudoCtor",
"cajaVM",

// Early DOM pieces
@@ -554,6 +558,9 @@
// TODO test invocation on Function.prototype itself
G.tuple(G.value(THIS), G.tuple()));

+ // Iterators. (There is no common superclass of iterators)
+ argsByProp('next', freshResult(genMethod())); // common iterator
method
+
// ES6 Generators
if (cajaVM.anonIntrinsics.GeneratorFunction) {

functionArgs.set(RefAnyFrame('cajaVM.anonIntrinsics.GeneratorFunction'),
@@ -580,6 +587,7 @@
aGeneratorFunction);

obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction.prototype,
aGenerator);
+ obtainInstance.define(aGeneratorFunction, aGenerator);

var generatorIteratorPrototype =
cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype;
@@ -588,7 +596,6 @@
// subtype of Iterator".
functionArgs.set(
Ref.all(
- Ref.is(generatorIteratorPrototype.next),
Ref.is(generatorIteratorPrototype['return']),
Ref.is(generatorIteratorPrototype['throw'])),
// fresh because it returns { value: ..., done: ... }
@@ -598,6 +605,34 @@
Ref.is(generatorIteratorPrototype['throw']), true);
}

+ // Non-generator iterators
+ var iteratorSymbol;
+ try {
+ // Symbols are not yet whitelisted
+ iteratorSymbol = directAccess.evalInGuestFrame('Symbol.iterator');
+ } catch (e) { console.warn('Symbol.iterator missing'); }
+ // Create mock ctors for the objects which are only exposed as anon
+ // prototypes. This is a workaround for Context not having an inherent
+ // understanding of constructorless prototypes.
+ [
+ {name: 'ArrayIteratorPrototype', o: cajaVM.def([1, 2])},
+ {name: 'StringIteratorPrototype', o: "ab"},
+ ].forEach(function(info, index) {
+ if (cajaVM.anonIntrinsics[info.name]) {
+ function scanPseudoCtor() {
+ var iterator = info.o[iteratorSymbol]();
+ expectedUnfrozen.mark(Ref.is(iterator));
+ return iterator;
+ }
+ scanPseudoCtor.prototype = cajaVM.anonIntrinsics[info.name];
+ cajaVM.def(scanPseudoCtor);
+ // Override is-actually-an-instance check.
+ obtainInstance.define(scanPseudoCtor, new scanPseudoCtor());
+ argsByIdentity(scanPseudoCtor, genCall());
+ window[info.name + '_ScanPseudoCtor'] = scanPseudoCtor;
+ }
+ });
+
argsByIdentity(cajaVM.anonIntrinsics.ThrowTypeError, genAllCall());
expectedAlwaysThrow.setByIdentity(
cajaVM.anonIntrinsics.ThrowTypeError, true);
@@ -1112,12 +1147,13 @@
expectedAlwaysThrow.setByIdentity(o, true);
});

- // NodeList and friends (currently have no exported type)
+ // NodeList, DOMTokenList and friends (currently have no exported type)
argsByProp('item', genMethod(genSmallInteger));
argsByProp('namedItem', genMethod(genString));
argsByProp('add', genMethod(genString));
argsByProp('remove', genMethod(genString));
argsByProp('toggle', genMethod(genString));
+ argsByProp('set value', genMethod(genString));

// Forms
argsByProp('submit', G.none);

[fel...@gmail.com]

lgtm.

meta-comment: I've already forgotten most of what I know about the
scanner, and I'm not finding it easy to reconstruct that understanding,
so this is just a fairly superficial review. The changes locally seem to
make sense, and I'm trusting that you understand the implications of the
change better than me.

If you'd like me to do a deeper review, I can find some time tomorrow.

https://codereview.appspot.com/297960043/

[kpr...@google.com]

On 2016/04/28 19:43:52, felix8a wrote:
> lgtm.

> meta-comment: I've already forgotten most of what I know about the
scanner, and
> I'm not finding it easy to reconstruct that understanding, so this is
just a
> fairly superficial review. The changes locally seem to make sense, and
I'm
> trusting that you understand the implications of the change better
than me.

“Everyone knows that debugging is twice as hard as writing a program in
the first place. So if you're as clever as you can be when you write it,
how will you ever debug it?”

I'm fairly confident that this change is a strict improvement. It
introduces a kludge (the pseudo-ctors) but everything else is
straightforward, and the kludge is less “clever” than my attempt to make
Context directly understand the iterator prototypes.

https://codereview.appspot.com/297960043/

[re...@codereview-hr.appspotmail.com]

* Fix bad function call in Context. This bug was not found before
because it only shows up when trying to report an error on this
type of scan context.
* Coverage/fixes for iterator.next() and *IteratorPrototype.
* Coverage/fixes for generators.
* Add missing coverage for DOMSettableTokenList.

https://codereview.appspot.com/297960043/

[kpr...@google.com]

New snapshot just moves a function declaration to where it needs to be
to obey ES5 strict rules (which Chrome has moved on from but Firefox
hasn't yet).

https://codereview.appspot.com/297960043/

[eri...@gmail.com]

still LGTM

https://codereview.appspot.com/297960043/

[eri...@gmail.com]

On 2016/04/28 22:06:17, MarkM wrote:
> still LGTM

Oops. wrong cl. nevermind

https://codereview.appspot.com/297960043/

[fel...@gmail.com]

lgtm

https://codereview.appspot.com/297960043/