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

Fix 'Ready' message sequence for Firefox ext disabled worker. #6938

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

yurydelendik
Copy link
Contributor

Fixes #6937

It's a little bit messy since we are handling async messages in sync style. We cannot just add e.g. setTimeout or Promises because in some cases we dispose original objects, e.g. arrays in operator list, and cannot clone objects with JSON since some of them are typed arrays.

Proper and full solution will be to make our fake channel act like a real one.

@yurydelendik
Copy link
Contributor Author

I guess it's something random that was caused by this patch :/

@Snuffleupagus
Copy link
Collaborator

I guess it's something random that was caused by this patch :/

That's unfortunate. However, given that the disableWorker issue is limited to the Firefox versions of PDF.js (where it's not simple to disable workers in the first place, and definitely not recommended), I think that we can live with this at least for now.

Proper and full solution will be to make our fake channel act like a real one.

Obviously it'd be nice to get it fixed as described, but it might not need to be a too high priority.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/ac29a871ef2899f/output.txt

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/35725a20af3340d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/20c55f626f27018/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/35725a20af3340d/output.txt

Total script time: 19.75 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/20c55f626f27018/output.txt

Total script time: 20.99 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij
Copy link
Contributor

The previous failures are related to #6944. A fresh test run shows that all is fine now, so I would say that this patch is fine. @Snuffleupagus I would say that this is good to merge if the solution is fine and the original problem is fixed.

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Feb 1, 2016

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/ce2245434bed1e6/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Feb 1, 2016

From: Bot.io (Linux)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/2db5ae96f4a9522/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Feb 1, 2016

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/2db5ae96f4a9522/output.txt

Total script time: 14.01 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/2db5ae96f4a9522/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Feb 1, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/ce2245434bed1e6/output.txt

Total script time: 19.93 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@yurydelendik
Copy link
Contributor Author

Looks like a bad timing, and this patch might make thing less intermittent in terms of problem. However I still cannot reproduce that on real computer (ubuntu ff 46a2).

But I would like to push it to aurora. @Snuffleupagus can you check if it fixes the issue, let's open the bug and uplift that to aurora?

@Snuffleupagus
Copy link
Collaborator

Sorry for the delay here!
Just so I understand correctly: the current patch is still necessary even with PR #6948 (based on this comment)? If that is correct, please land this PR with r=me.

@yurydelendik
Copy link
Contributor Author

Just so I understand correctly: the current patch is still necessary even with PR #6948

Those patches independent and both address the same issue. However this one makes our testing angrier, but it's easy to use in the uplift to aurora. So I'm thinking to push it only to aurora, while we can, but don't accept it in our repo.

@Snuffleupagus
Copy link
Collaborator

So I'm thinking to push it only to aurora, while we can, but don't accept it in our repo.

This seems fine to me.

brendandahl added a commit that referenced this pull request Feb 9, 2016
Fix 'Ready' message sequence for Firefox ext disabled worker.
@brendandahl brendandahl merged commit d7e5935 into mozilla:master Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants