1304955 - Tagged template literals parses incorrect from startup cache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[zbinlin]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160922181628

Steps to reproduce:

New file /tmp/test.js:
```javascript
function foo(val) {
	return val;
}

console.assert("bar" === foo(`bar`));
console.assert("bar" === (foo`bar`)[0]);
console.assert("bar" === foo(String.raw`bar`));
```

Load by `loadSubScriptWithOptions`:
```javascript
Services.scriptloader.loadSubScriptWithOptions("file:///tmp/test.js", {
    target: {},
    ignoreCache: false,
}); // load and cache

Services.scriptloader.loadSubScriptWithOptions("file:///tmp/test.js", {
    target: {},
    ignoreCache: false,
}); // load from startup cache
```

Comment 1

[Tooru Fujisawa [:arai]]

Looks like XDR bug around callsiteobj.

regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=41d8277af19246308bdf51bf0cd60da3498681b1&tochange=5c6582975b9ddaac43260767af201294849ac9a6

so this seems to be from bug 1283334.


simplified testcase for js shell:
  var code = cacheEntry("assertEq('bar', String.raw`bar`);");
  var g = newGlobal({ cloneSingletons: true });
  evaluate(code, { global: g, saveBytecode: true });
  evaluate(code, { global: g, loadBytecode: true });

the following code shows that different object is passed to function:
  var code = cacheEntry("(x => x.toSource())`bar`;");
  var g = newGlobal({ cloneSingletons: true });
  print(evaluate(code, { global: g, saveBytecode: true })); // ["bar"]
  print(evaluate(code, { global: g, loadBytecode: true })); // [,]

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Leo, is this something we can fix in 51?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Review commit: https://reviewboard.mozilla.org/r/87060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87060/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Not sure what the best way is to test this, any pointers arai?

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

https://reviewboard.mozilla.org/r/87060/#review86120

I'd forward this review to jandem.

About test, code in comment #1 will work.
  var code = cacheEntry("assertEq('bar', String.raw`bar`);");
  var g = newGlobal({ cloneSingletons: true });
  evaluate(code, { global: g, saveBytecode: true });
  evaluate(code, { global: g, loadBytecode: true });

or, change the following test to check the value of tagged template.
https://hg.mozilla.org/mozilla-central/annotate/f40960c63bfac865d510ec9da42eeed74c384082/js/src/jit-test/tests/xdr/trivial.js#l40
> // code call site object
> test = "function f(a) { return a; }; f`a${4}b`;";
> evalWithCache(test, { assertEqBytecode: true, checkFrozen: true});

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87060/diff/1-2/

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

https://reviewboard.mozilla.org/r/87060/#review86552

Thanks for fixing this. r=me with comments below addressed.

::: js/src/jit-test/tests/xdr/tagged-template-literals.js:1
(Diff revision 2)
> +var code = cacheEntry("assertEq('bar', String.raw`bar`);");

Please also add a test based on the second test in comment 1, and assert the array is what we expect.

::: js/src/jsobj.cpp:1151
(Diff revision 2)
>  
>  static bool
>  GetScriptArrayObjectElements(JSContext* cx, HandleObject obj, MutableHandle<GCVector<Value>> values)
>  {
>      MOZ_ASSERT(!obj->isSingleton());
>      MOZ_ASSERT(obj->is<ArrayObject>() || obj->is<UnboxedArrayObject>());

We should assert the array has no sparse elements: MOZ_ASSERT(!obj->isIndexed());

Comment 8

[Jan de Mooij [:jandem]]

Regression, we should fix this before it's released.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87060/diff/2-3/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87060/diff/3-4/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=297fd8e651a6

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49246df91b04
Call site objects now store their elements as dense after bug 1283334. r=jandem

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49246df91b04

Comment 14

[Gerry Chang [:gchang]]

Track 51+ as regression.

Hi :emilio,
Since this also affects 51, do you think the patch is worth uplifting to 51?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Approval Request Comment
[Feature/regressing bug #]: bug 1283334
[User impact if declined]: JS regression.
[Describe test coverage new/current, TreeHerder]: Automated tests added in the patch.
[Risks and why]: Low, tested negative-diff fix.
[String/UUID change made/needed]: None

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Thanks for reminding me Gerry, I meant to have done this before :)

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8802764 [details]
Bug 1304955: Call site objects now store their elements as dense after bug 1283334.

Fix a regression and add tests. Take it in 51 aurora.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ec527fde116

Comment 19

[Marcia Knous [:marcia]]

Tracking 52- since this is now fixed on central.