New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 837820 link

Starred by 1 user
Status: Verified
Owner:
ntfschr@chromium.org
Closed: May 2018
Cc:
█ gsennton@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Do we need to convert parameter classes across ClassLoaders?

Project Member Reported by ntfschr@chromium.org, Apr 27 2018 
Right now, our dupeMethod() logic looks like:

 1. Fetch the delegate's declaring class (which will be the boundary interface, in whichever ClassLoader created the delegate)
 2. Fetch the classes of the parameter list (which will classes visible in the current classloader)
 3. For each class in the parameter list, get the version of that class defined by the delegate classloader [1]
 4. Return the corresponding method from the delegate class

Do we actually need step (3)? This step converts a parameter type named "Foo" into a "Foo" in the delegate's classloader. If we pass standard Java types (e.g., String) or Frameworks types (e.g., WebView), then both Foo classes are identical (they're defined in the Frameworks classloader).

Step (3) is relevant only if we have 2 classes named Foo, defined in both classloaders. The only classes defined in this manner are BoundaryInterfaces, and our convention is to always pass InvocationHandlers instead of BoundaryInterfaces. Do we intend to pass BoundaryInterfaces in the future, or can we simplify this method (perhaps adding a comment explicitly forbidding passing BoundaryInterfaces as params)?

gsennton@ I can handle the simplification, but I wanted to check with you before doing so. Am I missing some edge case?

[1] https://cs.chromium.org/chromium/src/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/BoundaryInterfaceReflectionUtil.java?l=29-37&rcl=c89d896926d06df6f308438b2e64b635594bed00

Comment 1 by gsennton@chromium.org, Apr 30 2018 

Yeah I think you're right - because we can't pass classes that only exist on one side we shouldn't need to convert the parameter classes (unless the Class.getDeclaredMethod(methodName, parameters) method needs you to pass method parameter classes specifically from the correct classloader). 

The only reason (AFAICT) we would want dupeMethod to convert method parameters and return values is, as you say, to passing boundary interfaces across the boundary. But we still need to use the Proxy class to convert a boundary interface on one side to the corresponding boundary interface on the other side. So if we want to pass a boundary interface across the boundary we need to convert the class implementing the boundary interface before we pass it across.
If we want to do so we should do it for the entire API surface.
IMO it's nicer to pass InvocationHandlers across because then you know on both sides that you have to perform a conversion (between boundary interface and invocationhandler) - if you pass a boundary interface it is not obvious that you need to convert your boundary interface implementation into one that is recognized by the classloader on the other side of the boundary.


I don't think we're missing any edge-cases here: if you perform the change and the support library runs/works fine then this should be fine :)
Project Member

Comment 2 by bugdroid1@chromium.org, May 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f82aa8257d464e8cdc0a4169faea586d2af38c46

commit f82aa8257d464e8cdc0a4169faea586d2af38c46
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri May 11 22:20:29 2018

AW: do not convert support lib param types

No expected change to behavior, but this should have a slight
performance boost to the support library.

Previously, for each method call on a boundary interface, we would
iterate over method parameter types and convert each type to the
corresponding type visible by delegateLoader. This work is redundant,
because we only ever pass pre-L frameworks types (e.g., WebView, String,
InvocationHandler) and primitives.

This work would not be redundant if we passed BoundaryInterfaces as
parameters to some BoundaryInterface methods, however we don't ever
intend to do so.

Bug:  837820 
Test: ./gradlew :webkit:connectedAndroidTest
Change-Id: Icd3bfc620a45f9acd437da514ae5a02b3d8a6b02
Reviewed-on: https://chromium-review.googlesource.com/1054122
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558037}
[modify] https://crrev.com/f82aa8257d464e8cdc0a4169faea586d2af38c46/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/BoundaryInterfaceReflectionUtil.java

Comment 3 by ntfschr@chromium.org, May 11 2018

Status: Verified (was: Assigned)
Closing this. No manual verification necessary from the QA team--I've verified this manually before landing.
►

Sign in to add a comment