1693027 - ScriptPreloader discards correctly decoded script incorrectly.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

In ScriptPreloader, when doing main thread decode we do the following:

  JS::RootedScript script(cx);
  if (JS::DecodeScript(cx, options, Range(), &script)) {
    mScript.Set(script);

    if (mCache.mSaveComplete) {
      FreeData();
    }
  }

The problem here is that JS::DecodeScript returns a JS::TranscodeResult, which has TranscodeResult_Ok = 0. In effect, regardless of whether or not we've successfully main-thread decoded a buffer, we will never set mScript to anything but null (assuming here that on failure cases the outparam isn't modified).

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1693027 - Explicitly check for JS::TranscodeResult_Ok r?kmag

JS::TranscodeResult_Ok is defined to be zero; this means that during synchronous decode
of a script, regardless of if we succeed or fail we ultimately return a nullptr script.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1693027 - Convert TranscodeResult to a strongly typed enum r?arai!,kmag!

Depends on D105303

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Performance testing the first patch above (D105303) shows low impact in most tests, but a medium confidence 1% regression on four a11y tests .

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

(An alternate solution to this bug would be to just toss the main-thread decode entirely, and delete the lines highlighted in the original summary)

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Updated performance result.

When I first submitted D105303 I had accidentally recapitulated the original bug in rewriting the expression.

These perf results show no impact; this problem was originally detected by seeing scripts that should have loaded from the ScriptPreloader instead coming from the StartupCache. Must not be a huge difference in which cache is serving the request, so long as one of them does?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6113f461cb8
Explicitly check for JS::TranscodeResult_Ok r=kmag
https://hg.mozilla.org/integration/autoland/rev/1c32e8415dd6
Convert TranscodeResult to a strongly typed enum r=arai,kmag

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b6113f461cb8
https://hg.mozilla.org/mozilla-central/rev/1c32e8415dd6

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #28880 (as of Tue, 23 Feb 2021 04:35:50 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
10%	cpstartup	content-process-startup	windows10-64-shippable-qr	e10s stylo webrender	112.79 -> 101.42
9%	cpstartup	content-process-startup	windows10-64-shippable-qr	e10s stylo webrender-sw	112.04 -> 102.25
9%	cpstartup	content-process-startup	windows10-64-shippable-qr	e10s stylo webrender-sw	112.75 -> 103.17

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28880