Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Regression] Move the super-call in the PredictorStream-constructor to prevent errors (PR 13303) #13337

Merged
merged 1 commit into from May 5, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

My apologies for breaking this; thankfully PR #13303 hasn't reach mozilla-central yet.

It's (obviously) necessary to initialize a PredictorStream-instance fully, since otherwise breakage may occur if there's errors during the actual stream parsing.
To reproduce this issue, try opening the PDF document from issue #13051 locally and observe the following message in the console:

Warning: Invalid stream: "ReferenceError: this hasn't been initialised - super() hasn't been called"

…or to prevent errors (PR 13303)

*My apologies for breaking this; thankfully PR 13303 hasn't reach mozilla-central yet.*

It's (obviously) necessary to initialize a `PredictorStream`-instance fully, since otherwise breakage may occur if there's errors during the actual stream parsing.
To reproduce this issue, try opening the PDF document from issue 13051 locally and observe the following message in the console:
```
Warning: Invalid stream: "ReferenceError: this hasn't been initialised - super() hasn't been called"
```
@calixteman
Copy link
Contributor

calixteman commented May 5, 2021

Is it possible to add a test using a PredictorSteam ? and/or maybe transform some warnings into errors when running tests.

@Snuffleupagus
Copy link
Collaborator Author

It actually turns out that I cannot reproduce this in any build, but only locally in gulp server mode which suggests that this is some kind of SystemJS/Babel related problem.
Hence this is a) a lot less severe than I initially thought, and b) not really possible/necessary to test.


Note that the intention is to get rid of SystemJS for local development, see PR #12563, however that's unfortunately still blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@calixteman calixteman merged commit 5296119 into mozilla:master May 5, 2021
@Snuffleupagus Snuffleupagus deleted the PredictorStream-super branch May 5, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants