Skip to content
This repository has been archived by the owner on Jul 3, 2019. It is now read-only.

Add stub implementation of ExternalInterface to shell and inspector player #2293

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tschneidereit
Copy link
Contributor

Review on Reviewable

In many cases, getting one message per run per unique warning is enough and everything above that is just spam.
…layer. r=yury

Many SWFs are thrown off by exceptions caused by missing ExternalInterface, because they just expect that to be available. With this patch, they get to continue running, at least up until the point where they do an `ExternalInterface.call` and expect something to be returned other than `undefined`.
…line

The file got renamed, but apparently the old version persisted.
@tschneidereit
Copy link
Contributor Author

/botio test

@shumway-bot
Copy link
Contributor

From: Bot.io (Main)


Received

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

Live output at: http://areweflashyet.com:8081/466f0d9968fbdd8/output.txt

@shumway-bot
Copy link
Contributor

From: Bot.io (Main)


Success

Full output at http://areweflashyet.com:8081/466f0d9968fbdd8/output.txt

Total script time: 11.85 mins

  • Lint: Passed
  • Reference tests: Passed
  • Trace tests: Passed
  • AVM2 tests: Passed
  • AVM1 trace tests: Passed
  • Perf tests: Passed

@yurydelendik
Copy link
Contributor

I don't see why this needed -- the SWF movie in most of the cases check for presence of ExternalInterface and properly handle the exception.

@yurydelendik
Copy link
Contributor

If it's for mocking the ExternalInterface, could you add it as pref'd off options?

@tschneidereit
Copy link
Contributor Author

I don't see why this needed -- the SWF movie in most of the cases check for presence of ExternalInterface and properly handle the exception.

That's the thing: this is just not true, sadly. In many, many cases, SWFs just assume that ExternalInterface is available.

If it's for mocking the ExternalInterface, could you add it as pref'd off options?

What's the advantage of having it pref'd off? When do you ever want the inspector or shell to die because of this instead of just logging a warning?

@yurydelendik
Copy link
Contributor

When do you ever want the inspector or shell to die because of this instead of just logging a warning?

In default environment, ExternalInterface.available returns false and fails on methods calls. So FP will fail as well (you can notice that when debug build of FP is used). Mocking the external interface, e.g. returning undefined on document.location.href call without raising exception, might send logic to the different path or trigger unrelated failures for other properly designed SWFs.

@tschneidereit
Copy link
Contributor Author

In default environment, ExternalInterface.available returns false and fails on methods calls. So FP will fail as well (you can notice that when debug build of FP is used). Mocking the external interface, e.g. returning undefined on document.location.href call without raising exception, might send logic to the different path or trigger unrelated failures for other properly designed SWFs.

That is a good point to some extent. However, we have 127 exceptions in the ads report that're caused by SWFs not checking for availability of ExternalInterface. I'd bet a serious sum on this causing more problems than the scenario you're worried about. Do you have any idea for how to get the best of both worlds? If not, I think we should make the behavior introduced in this PR the default, and I'll add an option for turning it off.

@yurydelendik
Copy link
Contributor

Looks good with option to turn it on added -- test runner might turn that option from command line.


Reviewed 7 of 7 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@tbtlr
Copy link
Contributor

tbtlr commented Oct 14, 2015

@tschneidereit please rebase and land if we are still interesting in having this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants