Allow deferred intent when the screen is locked

chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java

Index: chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java b/chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java
new file mode 100644
index 0000000000000000000000000000000000000000..2a4c0b6e962f992a55fd72ea2ab78f7d08b612b1
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java
@@ -0,0 +1,56 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser;
+
+import android.content.BroadcastReceiver;
+import android.content.Context;
+import android.content.Intent;
+
+import org.chromium.base.VisibleForTesting;
+
+/**
+ * Broadcast receiver when the screen is unlocked from pin, pattern, or password.

[Maria, 2016/12/13 21:36:09]

I would add a note that this class handles exactly one intent unlock + dispatch
and then is expected to be destroyed and cannot be reused right now
(mShouldFireIntent is never reset)

[Sherry, 2016/12/13 22:13:54]

Will do.

+ */
+public class KeyguardBroadcastReceiver extends BroadcastReceiver {

[Ted C, 2016/12/13 22:47:13]

based on my comment in the other file about unregistering itself based on a
timer.

I wonder if we could encapsulate all of the logic in this file.  Back of napkin
thinking is that updateDeferredIntent would be the only public method.  Passing
null would unregister.  A non-null value would register the receiver if needed
(and post a unregister task to take place after the valid duration).

If we unregister based on the timeout, we no longer need the
hasValidDeferredIntent.  We just check that it is non-null.

[Sherry, 2016/12/14 00:54:07]

It sounds ideal, however, that will have this class keep the context as a member
so that it could register/unregister the receiver freely. It will cause more
problems since ChromeActivity holds the instance of IntentHandler, IntentHandler
holds the instance of the receiver, the receiver holds the instance of the
Context which could potentially be ChromeActivity. If possible, I would prefer
the receiver never holds the Context instance as a member.

What's more, another backwards of doing so would be the receiver tries to
register/unregister itself to the context. However, it is hard to track the
right instance registered/unregistered without keeping a track of the receiver
instance. It might be more messy to keep the receiver instance as a static
member.

[Ted C, 2016/12/14 19:55:06]

Can you not use the application context (ContextUtils.getApplicationContext())?

Then you are isolated from GC issues.
Huh?  This class is the instance so there is certainly no tracking that needs
to be done.

After discussing this with Maria, we both agree that my proposed approach is
what we want to see.

Some things I think we should do:

1.) Not name this as a <...>Receiver (or even Keyguard).  I would name
this something like DelayedScreenLockIntentHandler (something that
makes it clearer that it is purely here to handle intents while
the screen is locked/off).  That name could likely be refined.

2.) IntentHandler creates an instance and stores it.  It calls
updateIntent(null), updateIntent(validIntent) as the only two
entry points.

3.) Internally, this class registers the receiver.  If the application
context does not work, the passing in a context to updateIntent
is fine (and preferred).

4.) In the constructor, create a new Handler() object.  In update
intent w/ a valid intent, post a task to will unregister after
the timeout (you need to do Handler#removeCallbacks on subsequent
calls to update to prevent early unregistration).

In general, IntentHandler is complicated enough and any and all
effort should be done to isolate this logic into its own file.

[Sherry, 2016/12/14 22:25:46]

See my replies to each of your 4 points:
1.) Agree, I will update the name to DelayedScreenLockIntentHandler instead.
2.) I could move forward with that. However, I have some minor questions below:
   2.a) on updateIntent(null), should I unregister the receiver?
   2.b) if there are two different places unregister the receiver, do you
concern about the conflicts of unregister a receiver that is already
unregistered?
3.) Agree, it should not be a big deal.
4.) What will happen if the receiver was unregistered due to timeout and a new
deferred intent came. e.g. I fire an intent which creates the receiver instance
and registered to the context. Keep the screen locked. On timeout, the recevier
is unregistered from the context. Then fire another intent. The second intent
will never be handled since the receiver has been unregistered. But in my
expectation, the second intent should replace the first one and be handled
correctly when the screen is unlocked.
Besides, with current unregistraion solution, the worst case would be leaving
the receiver registered and almost do nothing. The cost of having it around
would be low. With the logic to unregister on timeout, it might add more
complexity to the logic and potentially introducing more corner cases that cause
problems.

In general, I will move all logic into the receiver and keep IntentHandler
clean. I still have concerns for the  proposed way of handling receiver
unregistration though.

[Sherry, 2016/12/16 21:10:59]

I have a plan to make the delayed task for unregistration work. It could be
something like:
class DelayedScreenLockIntentHandler {
  private boolean isRegistered = false;
  private synchronized void registerReceiver() {
     if (!isRegistered) {
        // register the receiver here
        isRegistered = true;
     }
  private synchronized void unregisterReceiver() {
     if (registered) {
        // unregister the receiver here
        isRegistered = false;
  }
}

And invoke register when updateDeferredIntent with a valid intent is triggered.
But it will actually register at most once.
unregisterReceiver will be triggered in three different ways:
- When onNewIntent() from IntentHandler.
- When onReceive() handled the deferred intent already.
- When timeout for the current deferredIntent. I'm going to use
Handler.postDelayed to post a delayed task for unregistration in
updateDeferredIntent as you proposed in 4.).

Since I introduced synchronized in the register/unregister function, it should
not cause any synchronous issue if the unregister is triggered from a different
thread.

[Sherry, 2016/12/16 23:00:40]

My bad, just realized Handler is running the task on the same thread where it
was created. Hence there should be no synchronize problems. Will remove the
synchronous keywords.

+    private static final int VALID_DEFERRED_PERIOD_MS = 10000;
+
+    private Intent mDeferredIntent;
+    private long mDeferredIntentCreatedTime;
+    private boolean mShouldFireIntent;
+
+    public KeyguardBroadcastReceiver(Intent deferredIntent) {
+        updateDeferredIntent(deferredIntent);
+        mShouldFireIntent = true;

[Ted C, 2016/12/13 22:47:13]

can use just use the null-ness of mDeferredIntent instead?

Seems like we just set the intent to null after we call startActivity and
everything should be good.

[Sherry, 2016/12/14 00:54:07]

That was my first intuition as well. I used to think it was not working as
expected since it would cause troubles in hasValidDeferredIntent where it checks
if the deferred intent is null. If mDeferredIntent was simply set to null in
this class, then in method IntentHandler.tryUnregisterLockscreenIntentReceiver
will not have its logic working correctly since hasValidDeferredIntent will
always return false.

But think about this solution again, it might lead to the actual behavior we
expect. Once the deferred intent was fired, there's no reason to make
hasValidDeferredIntent return true. Which lead to the results that
tryUnregisterLockscreenIntentReceiver will always unregister the receiver when
triggered from onNewIntent. And that would be the expectation where under any
situation, when onNewIntent was triggered, as long as mReceiver is not null, it
will be unregistered and dropped. The check of hasValidDeferredIntent became
unnecessary since it will always return false in this situation. But I may still
keep it in case this method was shared in other functions in the future where
checking the validity of deferred intent is still correct.

+    }
+
+    @Override
+    public void onReceive(Context context, Intent intent) {
+        assert Intent.ACTION_USER_PRESENT.equals(intent.getAction());
+
+        if (Intent.ACTION_USER_PRESENT.equals(intent.getAction())
+                && hasValidDeferredIntent(mDeferredIntent) && mShouldFireIntent) {
+            context.startActivity(mDeferredIntent);

[Ted C, 2016/12/13 22:47:13]

what context is this referring to?

In particular, I'm assuming this is not an activity context, so I think these
intents would need ensure they have NEW_TASK or NEW_DOCUMENT associated with
them.

Or maybe we don't save intents that don't include this?

[Sherry, 2016/12/14 00:54:07]

According to
https://developer.android.com/reference/android/content/BroadcastReceiver.htm...,
android.content.Intent), the context is where the receiver is running. It is not
the same one as the activity.

In the shouldIgnoreIntent method, the receiver will only be registered when the
intent is neither internal nor visible, which guards the deferred intent was
sent from external and not expecting internal context.

+            // Prevent the broadcast receiver from firing intent unexpectedly.
+            mShouldFireIntent = false;
+        }
+    }
+
+    public boolean hasValidDeferredIntent(final Intent intent) {

[Maria, 2016/12/13 21:36:09]

style: add javadoc to public methods

[Sherry, 2016/12/13 22:13:54]

Will do.

+        return mDeferredIntent != null
+                && System.currentTimeMillis() - mDeferredIntentCreatedTime

[Maria, 2016/12/13 21:36:09]

style: parenthesis around subtraction

[Sherry, 2016/12/13 22:13:54]

Will update.

+                        <= VALID_DEFERRED_PERIOD_MS
+                && intent == mDeferredIntent;
+    }
+
+    public void updateDeferredIntent(final Intent intent) {
+        mDeferredIntent = intent;
+        mDeferredIntentCreatedTime = System.currentTimeMillis();

[Maria, 2016/12/13 21:36:09]

For timing events use SystemClock.elapsedRealtime() instead -- it's guaranteed
to be monotonic.

[Sherry, 2016/12/13 22:13:54]

Will update.

+    }
+
+    @VisibleForTesting
+    void setCreateTimeForTesting(final long time) {
+        mDeferredIntentCreatedTime = time;
+    }
+}