Skip to content
This repository was 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
tschneidereit wants to merge 3 commits into
mozilla:masterfrom
tschneidereit:stub-externalinterface
Open

Add stub implementation of ExternalInterface to shell and inspector player#2293
tschneidereit wants to merge 3 commits into
mozilla:masterfrom
tschneidereit:stub-externalinterface

Conversation

@tschneidereit

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

/botio test

@shumway-bot

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@tschneidereit

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

tbtlr commented Oct 14, 2015

Copy link
Copy Markdown

@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