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

Issue 842695 link

Starred by 2 users
Status: Assigned
Owner:
█ tigero@google.com
Last visit > 30 days ago
Cc:
agrieve@chromium.org
tedc...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task



Sign in to add a comment

Enforce thread restrictions for annotated methods via bytecode rewriting

Project Member Reported by estevenson@chromium.org, May 14 2018 
Idea here is to add calls to ThreadUtils methods when thread annotations are present.

Specifically we'd want:
  * @UiThread -> ThreadUtils.assertOnUiThread()
  * @WorkerThread -> ThreadUtils.assertOnBackgroundThread()

@UiThread
void doUiMethod() {
  // Method body
}

becomes:

@UiThread
void doUiMethod() {
  ThreadUtils.assertOnUiThread();
  // Method body
}

We'd want to add a new ClassAdapter here: https://cs.chromium.org/chromium/src/build/android/bytecode/java/org/chromium/bytecode/ 

Probably isn't worthwhile converting existing uses of ThreadUtils to use annotations since there are so many (https://cs.chromium.org/search/?q=ThreadUtils.assertOnUiThread+lang:java&sq=package:chromium&type=cs) but we should at least send out a psa (and maybe prevent new usages?).

Comment 1 by estevenson@chromium.org, May 23 2018

Owner: tigero@google.com
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 13 

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

commit afe32ece3a686161401f2c552e859ef3bb338274
Author: Tiger Oakes <tigero@google.com>
Date: Fri Jul 13 17:52:40 2018

Added processor for @UiThread & @WorkerThread annotations

When a method has the @UiThread annotation, a call to
ThreadUtils.assertOnUiThread will be injected at the start of the method.
Similarily, @WorkerThread adds a call to ThreadUtils.assertOnBackgroundThread.
This allows for easily asserting that a method can only be used on the UI or
worker threads.

Bug: 842695
Change-Id: Ic53b5d06d6b0581a81f1dcce72b46e7ab42313c0
Reviewed-on: https://chromium-review.googlesource.com/1070592
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Commit-Queue: Tiger Oakes <tigero@google.com>
Cr-Commit-Position: refs/heads/master@{#574971}
[modify] https://crrev.com/afe32ece3a686161401f2c552e859ef3bb338274/build/android/bytecode/BUILD.gn
[modify] https://crrev.com/afe32ece3a686161401f2c552e859ef3bb338274/build/android/bytecode/java/org/chromium/bytecode/ByteCodeProcessor.java
[add] https://crrev.com/afe32ece3a686161401f2c552e859ef3bb338274/build/android/bytecode/java/org/chromium/bytecode/ThreadAssertionClassAdapter.java
[modify] https://crrev.com/afe32ece3a686161401f2c552e859ef3bb338274/build/android/gyp/bytecode_processor.py
[modify] https://crrev.com/afe32ece3a686161401f2c552e859ef3bb338274/build/config/android/internal_rules.gni

Comment 3 by tigero@google.com, Jul 13

Status: Fixed (was: Assigned)
Created the annotation processor behind a flag that's currently turned off.
Follow up issue is #863484
https://bugs.chromium.org/p/chromium/issues/detail?id=863484

Comment 4 by agrieve@chromium.org, Jul 20

Status: Available (was: Fixed)
Realized that the bytecode rewriter never finds any annotations because the annotations have Retention=Source (they don't appear in the .class files).

Idea #1: Abandon this idea
Idea #2: User our own annotations with Retention=class
Idea #3: Rewrite the support annotation bytecode to change their retention
Idea #4: Write an annotation processor to dump the annotations to a file while compiling, then read that when bytecode processing.

Comment 5 by benhenry@chromium.org, Aug 2

Status: Assigned (was: Available)
►

Sign in to add a comment