Refactor chromium's Linker class.

Refactor chromium's Linker class.

Preparation for creating an alternative non-crazy linker based
chromium Linker:

- Convert Linker from pure static into a singleton class.
- Split into an abstract base class containing (expected) common code,
and (for now) a single concrete subclass that implements the current
crazy linker based loading of libchrome.so.
- Update client code to handle the singleton getInstance() requirement
and class name changes.
- Modernize logging.

Later changes will introduce a second concrete Linker subclass that is
free of crazy linker code, for Android platform system linkers that
handle packed relocations, load from apk, and shared relro natively.

BUG=385553
Committed: https://crrev.com/908ec61cdcf5b82547596c930caf54f00b840955
Cr-Commit-Position: refs/heads/master@{#338182}

[simonb (inactive), 2015-07-07 18:18:28]

simonb@chromium.org changed reviewers:
+ petrcermak@chromium.org, rmcilroy@chromium.org

[rmcilroy, 2015-07-08 13:32:49]

Looks good, a couple of minor suggestions.

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:40: //
the browser process) on low-memory devices.
Orion had a CL which fixed this TODO but never got landed
(https://codereview.chromium.org/1101473006/). It might be worth dusting that CL
off and landing it as part of this refactor?

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:21: /*
nit - Javadoc comment '/**'

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:25:
class LegacyLinker extends Linker {
nit - add a private constructor and a getInstance static method to ensure this
is a singleton class.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:64:
protected HashMap<String, LibInfo> mLoadedLibraries = null;
Do we need this field it doesn't seem very useful (I think this may have
originally been added for the async library prefetch code but isn't used by that
any longer).

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:289:
public void useSharedRelros(Bundle bundle) {
add @Override to all functions which override superclass methods.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/Linker.java:154: private
static final String TAG = "cr.library_loader";
nit - maybe you could just make this public and have all the LibraryLoader,
LegacyLinker etc. set their TAG to Linker.TAG to ensure they are always the
same?

https://codereview.chromium.org/1224963002/diff/40001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:80:
Linker.getInstance().setMemoryDeviceConfig(memoryDeviceConfig);
nit - pull out a variable to avoid multiple calls to getInstance.

[petrcermak, 2015-07-08 14:21:49]

Looks great overall. Just a couple of style comments.

Thanks,
Petr

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:64:
protected HashMap<String, LibInfo> mLoadedLibraries = null;
I think we still need this to pass the RELROs later (see line 222).

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:306:
linker.loadLibrary(zipFilePath, libFilePath);
There's no need for the local variable here (you simply could do
"Linker.getInstance().loadLibrary(zipFilePath, libFilePath);").

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:377:
if (linker.isUsed()) {
ditto

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:405:
if (linker.isUsed()) {
ditto

[rmcilroy, 2015-07-08 14:26:30]

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:64:
protected HashMap<String, LibInfo> mLoadedLibraries = null;
On 2015/07/08 14:21:48, petrcermak wrote:
> I think we still need this to pass the RELROs later (see line 222).

Your right, sorry. Ignore me...

[simonb (inactive), 2015-07-08 16:46:42]

Updates in patch set 4.

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:40: //
the browser process) on low-memory devices.
On 2015/07/08 13:32:48, rmcilroy wrote:
> Orion had a CL which fixed this TODO but never got landed
> (https://codereview.chromium.org/1101473006/). It might be worth dusting that
CL
> off and landing it as part of this refactor?

We probably should land it at some point (and this refactor makes that landing
harder), but I'd like to keep mechanical refactoring separate from behaviour
changes.  There's doubtless some cargo-cult in here, but for now leaving it
undisturbed reduces the chance of unpleasant surprises later on.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:21: /*
On 2015/07/08 13:32:48, rmcilroy wrote:
> nit - Javadoc comment '/**'

Done.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:25:
class LegacyLinker extends Linker {
On 2015/07/08 13:32:48, rmcilroy wrote:
> nit - add a private constructor and a getInstance static method to ensure this
> is a singleton class.

It inherits getInstance() from the singleton abstract base Linker.  LegacyLinker
is package-private, which I think should provide enough guarantee against
accidental instantiation from outside.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:64:
protected HashMap<String, LibInfo> mLoadedLibraries = null;
On 2015/07/08 14:26:29, rmcilroy wrote:
> On 2015/07/08 14:21:48, petrcermak wrote:
> > I think we still need this to pass the RELROs later (see line 222).
> 
> Your right, sorry. Ignore me...

Done!

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:289:
public void useSharedRelros(Bundle bundle) {
On 2015/07/08 13:32:48, rmcilroy wrote:
> add @Override to all functions which override superclass methods.

Done.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:306:
linker.loadLibrary(zipFilePath, libFilePath);
On 2015/07/08 14:21:48, petrcermak wrote:
> There's no need for the local variable here (you simply could do
> "Linker.getInstance().loadLibrary(zipFilePath, libFilePath);").

Done.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:377:
if (linker.isUsed()) {
On 2015/07/08 14:21:48, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:405:
if (linker.isUsed()) {
On 2015/07/08 14:21:48, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/Linker.java:154: private
static final String TAG = "cr.library_loader";
On 2015/07/08 13:32:48, rmcilroy wrote:
> nit - maybe you could just make this public and have all the LibraryLoader,
> LegacyLinker etc. set their TAG to Linker.TAG to ensure they are always the
> same?

Or just protected.  Unfortunately, anything other than 'private static final'
prompts an annoying warning from PRESUBMIT.py.  See log_decl_pattern in
PRESUBMIT.py.

https://codereview.chromium.org/1224963002/diff/40001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:80:
Linker.getInstance().setMemoryDeviceConfig(memoryDeviceConfig);
On 2015/07/08 13:32:48, rmcilroy wrote:
> nit - pull out a variable to avoid multiple calls to getInstance.

Done.

[petrcermak, 2015-07-08 17:13:37]

LGTM.

Thanks,
Petr

[rmcilroy, 2015-07-09 09:23:45]

lgtm, thanks.

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:40: //
the browser process) on low-memory devices.
On 2015/07/08 16:46:41, simonb wrote:
> On 2015/07/08 13:32:48, rmcilroy wrote:
> > Orion had a CL which fixed this TODO but never got landed
> > (https://codereview.chromium.org/1101473006/). It might be worth dusting
that
> CL
> > off and landing it as part of this refactor?
> 
> We probably should land it at some point (and this refactor makes that landing
> harder), but I'd like to keep mechanical refactoring separate from behaviour
> changes.  There's doubtless some cargo-cult in here, but for now leaving it
> undisturbed reduces the chance of unpleasant surprises later on.

Sure, I wasn't suggesting landing it with this CL, I just wanted to point it out
for followup in a later CL.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:25:
class LegacyLinker extends Linker {
On 2015/07/08 16:46:42, simonb wrote:
> On 2015/07/08 13:32:48, rmcilroy wrote:
> > nit - add a private constructor and a getInstance static method to ensure
this
> > is a singleton class.
> 
> It inherits getInstance() from the singleton abstract base Linker. 
LegacyLinker
> is package-private, which I think should provide enough guarantee against
> accidental instantiation from outside.

Ok sounds fine.

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/1224963002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/Linker.java:154: private
static final String TAG = "cr.library_loader";
On 2015/07/08 16:46:42, simonb wrote:
> On 2015/07/08 13:32:48, rmcilroy wrote:
> > nit - maybe you could just make this public and have all the LibraryLoader,
> > LegacyLinker etc. set their TAG to Linker.TAG to ensure they are always the
> > same?
> 
> Or just protected.  Unfortunately, anything other than 'private static final'
> prompts an annoying warning from PRESUBMIT.py.  See log_decl_pattern in
> PRESUBMIT.py.

Thats annoying, ohh well, looks fine as is.

https://codereview.chromium.org/1224963002/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
(right):

https://codereview.chromium.org/1224963002/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java:23: *
of how this class is supposed to be used in Linker.java.
nit - just move this to the javadoc comment?

[simonb (inactive), 2015-07-09 10:08:25]

simonb@chromium.org changed reviewers:
+ tedchoc@chromium.org

[simonb (inactive), 2015-07-09 10:08:26]

+tedchoc for content OWNERS review.

[Ted C, 2015-07-09 17:18:45]

content - lgtm

[simonb (inactive), 2015-07-09 18:03:02]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2015-07-09 18:05:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224963002/60001

[commit-bot: I haz the power, 2015-07-09 18:19:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 18:19:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[simonb (inactive), 2015-07-09 22:01:29]

simonb@chromium.org changed reviewers:
+ thakis@chromium.org

[simonb (inactive), 2015-07-09 22:01:30]

Sigh.  +thakis for base OWNERS review.

[Nico, 2015-07-09 22:21:31]

Sigh.  lgtm

[simonb (inactive), 2015-07-09 22:41:03]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2015-07-09 22:41:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224963002/60001

[commit-bot: I haz the power, 2015-07-09 23:24:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-09 23:25:36]

Patchset 4 (id:??) landed as
https://crrev.com/908ec61cdcf5b82547596c930caf54f00b840955
Cr-Commit-Position: refs/heads/master@{#338182}