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

feat: add 'ensureWebviewsHavePages' cap #553

Merged
merged 3 commits into from Jul 16, 2019
Merged

feat: add 'ensureWebviewsHavePages' cap #553

merged 3 commits into from Jul 16, 2019

Conversation

@jlipps
Copy link
Member

@jlipps jlipps commented Jul 10, 2019

This PR fixes an odd situation which I have observed with Chrome-backed webviews on Android. Currently, Appium's code assumes that if it can find a webview-type process in /proc/net/unix, then a webview exists for that process.

I've noticed that this assumption is not always true. Sometimes, the process exists, and a Chrome remote debugger is even running for the process, but there are no active webview pages. In this case, Chromedriver will always fail to attach to the webview, resulting in an error like "unable to discover open pages".

This PR adds a new capability ensureWebviewsHavePages which, if true, attempts to verify that a webview actually has active pages before adding it to the list of contexts. This serves to prevent users from trying to connect to a webview which is guaranteed to fail, and which will result in a cryptic error message.

I've added this behind a capability because the only way I can see to achieve this is to send a request to one of the Chrome devtools HTTP endpoints. This involves running an adb command to forward a local port to the remote debugger port on the device, so we can make the necessary HTTP request, and then cleaning up the forwarded port afterwards. Thus it potentially adds overhead of time, and I'm also not certain that all webviews on all supported versions of Android support the Chrome devtools HTTP server. Once this feature is well-tested and proved useful, we could make this behavior opt-out rather than opt-in.

Because a local port is required for this feature, I've also added another capability, webviewDevtoolsPort, so that clients can specify a port to avoid port conflicts between sessions.

@jlipps jlipps requested a review from imurchie Jul 10, 2019
@mykola-mokhnach
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach commented Jul 10, 2019

Thanks @jlipps for looking into this.
Linking appium/appium#11999

lib/commands/context.js Outdated Show resolved Hide resolved
lib/webview-helpers.js Outdated Show resolved Hide resolved
lib/webview-helpers.js Outdated Show resolved Hide resolved
lib/webview-helpers.js Outdated Show resolved Hide resolved
lib/webview-helpers.js Outdated Show resolved Hide resolved
if (ensureWebviewsHavePages) {
logger.info('Retrieved potential webviews; will filter out ones with no active pages');
webviewProcs = await asyncfilter(webviewProcs, async (wp) => {
return await webviewHasPages(adb, wp, webviewDevtoolsPort);
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 10, 2019

Choose a reason for hiding this comment

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

return is redundant

Copy link
Member Author

@jlipps jlipps Jul 12, 2019

Choose a reason for hiding this comment

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

i don't think so, doesn't the filter method need to return true or false in order to do its job?

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 12, 2019

Choose a reason for hiding this comment

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

I mean

await asyncfilter(webviewProcs, async (wp) => await webviewHasPages(adb, wp, webviewDevtoolsPort))

should work just fine

Copy link
Member

@KazuCocoa KazuCocoa left a comment

🎉

@jlipps
Copy link
Member Author

@jlipps jlipps commented Jul 12, 2019

ok @mykola-mokhnach i endured the tedium of updating all the function comments to proper docstrings :-) and found an excuse to use reduce 🎉

mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

Array<string>

mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

nice

mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

Consider using typedef declarations for such object descriptions like in https://github.com/appium/appium-adb/blob/94a87c180c7b912de70a979cbbaa8f5ec0032c01/lib/tools/system-calls.js#L141. VSCode understands them and shows proper autofill proposals.

mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

= {} is redundant

jlipps
Copy link
Member

jlipps commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

I thought that too but ran into this:

> function test3(foo, {baz = null, junk = null}) { console.log({foo, baz, junk}); }
undefined
> test3()
TypeError: Cannot destructure property `baz` of 'undefined' or 'null'.
    at test3 (repl:1:15)

mykola-mokhnach
Copy link
Contributor

mykola-mokhnach commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

wow, this is unexpected. Perhaps, there is a bug in the transpiler logic. I would try reporting it to babel

jlipps
Copy link
Member

jlipps commented on 92606f6 Jul 12, 2019

Choose a reason for hiding this comment

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

kinda makes sense to me, actually. should the compiler know to default undefined to {}?

@mykola-mokhnach
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach commented Jul 12, 2019

it would be useful to have the new cap documented after the PR is merged

@jlipps jlipps merged commit d868591 into master Jul 16, 2019
4 checks passed
@jlipps jlipps deleted the jlipps-webview-pages branch Jul 16, 2019
@jlipps
Copy link
Member Author

@jlipps jlipps commented Jul 16, 2019

published in 4.18.0. will put in docs PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants