1139152 - Shumway no longer loads IMDb or Amazon videos

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

When I try to play an IMDb trailer in Nightly 39 from the imdb.com home page, the video player pops up, the progress bar shows data is being loaded, but the video does not play. I see this with the new profile and the bundled Shumway or latest extension. Using the latest extension with Aurora 38 or Firefox 36, the IMDb videos play correctly.

I don't see any obvious error messages in the browser console, but I see the following MPEG errors in stderr:

W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 572 looking for first track
W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 572 looking for first track
W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 572 looking for first track
W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 572 looking for first track
W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 92576 looking for first track
W/MPEG4Extractor(47109): Error -1004 parsing chuck at offset 93634 looking for first track
ShumwayStreamConverter.js: Timeout during shumway instance initialization

Comment 1

[Chris Peterson [:cpeterson]]

I used mozregression to bisect this regression to h4writer's IonMonkey optimizations:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=928ec762c672&tochange=490afdad9ba1

Comment 2

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Hannes, this Shumway video problem looks like a regression from your IonMonkey optimizations in bug 994016.

Comment 3

[Yury Delendik (:yury)]

To run Shumway without plugin:

http://areweflashyet.com/shumway/examples/inspector/inspector.html?rfile=../Box2DFlashAS3/demo.swf

(Disabling Ion with 'javascript.options.ion' fixes Shumway)

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Patch

This fixes it locally.

I will make a testcase tomorrow. Time for bed now.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8572255 [details] [diff] [review]
Patch

Review of attachment 8572255 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, r=me if you can add a test that fails without the patch.

We should probably also handle JSTYPE_BOOLEAN right?

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Make sure typeset is correct when using redefine.

This makes it easier to find similar issues.
(This currently introduces 5 new failures. Investigating them now.)

Comment 7

[Hannes Verschore [:h4writer]]

Adding gkw and decoder. After this patch they can replace --ion-check-range-analysis with --ion-fuzzer-checks. Which is a collection of checks enabled that we want fuzzers to check. (Currently this enables --ion-check-range-analysis and --ion-extra-checks).

I'll ping both when this land to let them know.

Comment 8

[Hannes Verschore [:h4writer]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6dfab0f84d52

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Make sure typeset is correct when using redefine.

The issue was that some LIRs first added "redefine" and afterwards executed a potential guard (which bailed out for some types). So we were seeing indeed incorrect types.
Solved by first adding the guard and afterwards the redefine.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6dfab0f84d52

Comment 11

[Hannes Verschore [:h4writer]]

Still a patch that needs to get landed in here. So reopening.

Comment 12

[Chris Peterson [:cpeterson]]

We don't need to track for 39 because Hannes already landed a fix.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> Adding gkw and decoder. After this patch they can replace
> --ion-check-range-analysis with --ion-fuzzer-checks. Which is a collection
> of checks enabled that we want fuzzers to check. (Currently this enables
> --ion-check-range-analysis and --ion-extra-checks).

I think this is overkill. Can we just rename --ion-check-range-analysis to --ion-extra-checks, and have it do both?

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8572670 [details] [diff] [review]
Make sure typeset is correct when using redefine.

Review of attachment 8572670 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks for adding this.

::: js/src/jit/shared/Lowering-shared-inl.h
@@ +157,5 @@
>      if (gen->optimizationInfo().registerAllocator() == RegisterAllocator_LSRA)
>          add(new(alloc()) LNop);
>  }
>  
> +

Nit: don't add an extra newline.

@@ +480,5 @@
> +#endif
> +}
> +
> +void
> +LIRGeneratorShared::redefine(MDefinition *def, MDefinition *as)

Any reason for moving the function down in this file? It makes it harder to do hg annotate/blame later. Also, if we're moving it, we might as well move it to the .cpp file, it's 60 lines of code and not super hot.

::: js/src/shell/js.cpp
@@ +5911,5 @@
> +        jit::js_JitOptions.runExtraChecks = true;
> +
> +    if (op.getBoolOption("ion-fuzzer-checks")) {
> +        jit::js_JitOptions.checkRangeAnalysis = true;
> +        jit::js_JitOptions.runExtraChecks = true;

I still think two new flags is not necessary, we can have the extra-checks and check-range-analysis flags (and have them both in tests.py). The fuzzers just pick flags at random so I think there's no need to combine these two with a third flag.

Comment 15

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf93416b39a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cdf93416b39a
https://hg.mozilla.org/mozilla-central/rev/23d74b0e4e21

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Added support for --ion-extra-checks in fuzzing rev 487042e873e0.