Closed Bug 1139152 Opened 5 years ago Closed 5 years ago

Shumway no longer loads IMDb or Amazon videos

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- unaffected
firefox39 --- fixed

People

(Reporter: cpeterson, Assigned: h4writer)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

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
I used mozregression to bisect this regression to h4writer's IonMonkey optimizations:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=928ec762c672&tochange=490afdad9ba1
Blocks: 994016
Keywords: reproducible
[Tracking Requested - why for this release]:

Hannes, this Shumway video problem looks like a regression from your IonMonkey optimizations in bug 994016.
Flags: needinfo?(hv1989)
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)
Attached patch PatchSplinter Review
This fixes it locally.

I will make a testcase tomorrow. Time for bed now.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8572255 - Flags: review?(jdemooij)
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?
Attachment #8572255 - Flags: review?(jdemooij) → review+
This makes it easier to find similar issues.
(This currently introduces 5 new failures. Investigating them now.)
Attachment #8572623 - Flags: review?(jdemooij)
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.
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.
Attachment #8572623 - Attachment is obsolete: true
Attachment #8572623 - Flags: review?(jdemooij)
Attachment #8572670 - Flags: review?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/6dfab0f84d52
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Still a patch that needs to get landed in here. So reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: shumway-m3
We don't need to track for 39 because Hannes already landed a fix.
Component: Shumway → JavaScript Engine
Product: Firefox → Core
Target Milestone: Firefox 39 → ---
(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 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.
Attachment #8572670 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/cdf93416b39a
https://hg.mozilla.org/mozilla-central/rev/23d74b0e4e21
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Added support for --ion-extra-checks in fuzzing rev 487042e873e0.
Duplicate of this bug: 1008111
You need to log in before you can comment on or make changes to this bug.