[Android WebView] Synthesize a fake page loading event on page source modification (Re-land)

[Android WebView] Synthesize a fake page loading event on page source modification (Re-land)

When a script modifies page source of a non-committed page, we need to
notify clients, so they can update the URL bar to avoid confusion.

New logic since the last attempt (https://codereview.chromium.org/924833003/):
distinguish between a "vanilla" WebView state (basically, a newly created
WebView, where no loading attempts have been made) and an "attempted to
navigate" state. In the "vanilla" state, don't fire synthesized page loading
events to avoid confusing clients. This is safe, as WebView is guaranteed to be
on a blank page.

Implementation note: we detect navigation attempts using
didStartProvisionalLoadForFrame WebContentsObserver event on the Java side. As
for popups AwWebContentsObserver gets re-attached from the original popup
WebView to the one provided by the client, notifications issued inbetween can be
missed on the Java side. To work around this, we assume that WebViews opened as
popups can never be in "vanilla" state (as they are anyway opened as a result of
navigation).

BUG=458569, 462213
TBR=davidben@chromium.org,tedchoc@chromium.org

Committed: https://crrev.com/b2803fd408c3709889645061fad42d3371024b93
Cr-Commit-Position: refs/heads/master@{#319061}

[mnaganov (inactive), 2015-03-02 13:44:42]

mnaganov@chromium.org changed reviewers:
+ creis@chromium.org, torne@chromium.org

[mnaganov (inactive), 2015-03-02 13:44:42]

Charile, Torne, please re-consider the anit-spoof patch.

[Charlie Reis, 2015-03-02 23:09:31]

Seems like there may be a simpler way than all the extra "vanilla" state
tracking.

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
&& mAwContents.getDidAttemptLoad()) {
I don't think we need this, do we?  It seems like
NavigationController::GetPendingEntry() would tell you whether there's an
in-progress load or not.  (DidStartProvisionalLoad creates a pending entry.)

Is there a reason you need the "has there ever been a load" state, or is
GetPendingEntry() sufficient?  We won't have a visible URL to spoof if there
isn't a pending entry.  (GetVisibleEntry will just return the last committed
entry in that case.)

[mnaganov (inactive), 2015-03-03 08:55:48]

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
&& mAwContents.getDidAttemptLoad()) {
On 2015/03/02 23:09:31, Charlie Reis wrote:
> I don't think we need this, do we?  It seems like
> NavigationController::GetPendingEntry() would tell you whether there's an
> in-progress load or not.  (DidStartProvisionalLoad creates a pending entry.)
> 
> Is there a reason you need the "has there ever been a load" state, or is
> GetPendingEntry() sufficient?  We won't have a visible URL to spoof if there
> isn't a pending entry.  (GetVisibleEntry will just return the last committed
> entry in that case.)

Indeed, my initial plan was to just use GetPendingEntry. Then I discovered that
WebContentsImpl::DidAccessInitialDocument discards the pending entry prior to
issuing  NavigationStateChanged notification, so when you check it here, it will
be null.

Then I tried to use the fact that NavigatorImpl::DidStartProvisionalLoad also
issues NavigationStateChanged notification immediately after creating the
pending entry, so I though I can catch it here, make a note to self via a
private flag, and then use this flag when I'll receive NavigationStateChanged
the second time due to a DOM mutation. But unfortunately, this approach was not
reliable due to asynchronous nature of notifications -- between the moment when
the pending entry has been set and the moment when this code receives
NavigationStateChanged, the pending entry could already had been discarded due
to a DOM mutation.

So I resorted to catching DidStartProvisionalLoadForFrame, which seems to work
reliably, as it's a synchronous callback.

[Torne, 2015-03-03 15:01:08]

Code in general LGTM; deferring to creis for final verdict about the approach
though :)

[Charlie Reis, 2015-03-03 20:12:50]

On 2015/03/03 08:55:48, mnaganov (cr) wrote:
>
https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
> File
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> (right):
> 
>
https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> && mAwContents.getDidAttemptLoad()) {
> On 2015/03/02 23:09:31, Charlie Reis wrote:
> > I don't think we need this, do we?  It seems like
> > NavigationController::GetPendingEntry() would tell you whether there's an
> > in-progress load or not.  (DidStartProvisionalLoad creates a pending entry.)
> > 
> > Is there a reason you need the "has there ever been a load" state, or is
> > GetPendingEntry() sufficient?  We won't have a visible URL to spoof if there
> > isn't a pending entry.  (GetVisibleEntry will just return the last committed
> > entry in that case.)
> 
> Indeed, my initial plan was to just use GetPendingEntry. Then I discovered
that
> WebContentsImpl::DidAccessInitialDocument discards the pending entry prior to
> issuing  NavigationStateChanged notification, so when you check it here, it
will
> be null.

Ah, that's unfortunate for this case, but yes, it seems safer to clear it.  (I
don't quite remember why that was necessary, given that GetVisibleEntry won't
return it once has_accessed_initial_document_ is true, but I'm hesitant to
change that without a closer look.)

> Then I tried to use the fact that NavigatorImpl::DidStartProvisionalLoad also
> issues NavigationStateChanged notification immediately after creating the
> pending entry, so I though I can catch it here, make a note to self via a
> private flag, and then use this flag when I'll receive NavigationStateChanged
> the second time due to a DOM mutation. But unfortunately, this approach was
not
> reliable due to asynchronous nature of notifications -- between the moment
when
> the pending entry has been set and the moment when this code receives
> NavigationStateChanged, the pending entry could already had been discarded due
> to a DOM mutation.

What do you mean by asynchronous?  NavigatorImpl::DidStartProvisionalLoad calls
NotifyChangedNavigationState, which synchronously calls
WebContentsDelegate::NavigationStateChanged.  We can't get a
DidAccessInitialDocument between DidStartProvisionalLoad and its
NavigationStateChanged call.  (Sorry, I'm not following this part.)


> So I resorted to catching DidStartProvisionalLoadForFrame, which seems to work
> reliably, as it's a synchronous callback.

Hmm, I suppose this will work, though it felt awkward.  Let me take another look
at the code after lunch to see if there's any ways to make it cleaner.

[mnaganov (inactive), 2015-03-03 21:21:14]

On 2015/03/03 20:12:50, Charlie Reis wrote:
> On 2015/03/03 08:55:48, mnaganov (cr) wrote:
> >
>
https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
> > File
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> > && mAwContents.getDidAttemptLoad()) {
> > On 2015/03/02 23:09:31, Charlie Reis wrote:
> > > I don't think we need this, do we?  It seems like
> > > NavigationController::GetPendingEntry() would tell you whether there's an
> > > in-progress load or not.  (DidStartProvisionalLoad creates a pending
entry.)
> > > 
> > > Is there a reason you need the "has there ever been a load" state, or is
> > > GetPendingEntry() sufficient?  We won't have a visible URL to spoof if
there
> > > isn't a pending entry.  (GetVisibleEntry will just return the last
committed
> > > entry in that case.)
> > 
> > Indeed, my initial plan was to just use GetPendingEntry. Then I discovered
> that
> > WebContentsImpl::DidAccessInitialDocument discards the pending entry prior
to
> > issuing  NavigationStateChanged notification, so when you check it here, it
> will
> > be null.
> 
> Ah, that's unfortunate for this case, but yes, it seems safer to clear it.  (I
> don't quite remember why that was necessary, given that GetVisibleEntry won't
> return it once has_accessed_initial_document_ is true, but I'm hesitant to
> change that without a closer look.)
> 
> > Then I tried to use the fact that NavigatorImpl::DidStartProvisionalLoad
also
> > issues NavigationStateChanged notification immediately after creating the
> > pending entry, so I though I can catch it here, make a note to self via a
> > private flag, and then use this flag when I'll receive
NavigationStateChanged
> > the second time due to a DOM mutation. But unfortunately, this approach was
> not
> > reliable due to asynchronous nature of notifications -- between the moment
> when
> > the pending entry has been set and the moment when this code receives
> > NavigationStateChanged, the pending entry could already had been discarded
due
> > to a DOM mutation.
> 
> What do you mean by asynchronous?  NavigatorImpl::DidStartProvisionalLoad
calls
> NotifyChangedNavigationState, which synchronously calls
> WebContentsDelegate::NavigationStateChanged.  We can't get a
> DidAccessInitialDocument between DidStartProvisionalLoad and its
> NavigationStateChanged call.  (Sorry, I'm not following this part.)
>

You are right -- these notifications are indeed synchronous. I must have mixed
them up with something else,
thanks for pointing this out!

Nevertheless, my attempt to use the pending entry which looked as below wasn't
working reliably for the popup test:

class AwWebContentsDelegateAdapter ... {
  private boolean mHadPendingEntry = false;
  ...
  public void navigationStateChanged(int flags) {
     if ((flags & InvalidateTypes.URL) != 0) {
       if (!mHadPendingEntry && ...getPendingEntry() != null) {
         mHadPendingEntry = true;
       }
       if (mHadPendingEntry && mAwContents.hasAccessedInitialDocument()) {
         ...
         // synthesize callbacks
         ...
       }
    }
  }
  ...
}

I'm not actually sure what was the underlying reason (at that point I
incorrectly assumed that this is because
the notifications are delivered asynchronously), but it surely didn't work as
expected in a reliable fashion.
Now I'm suspecting that this is because on the Java side AwContents swaps native
peers of Java classes when
the client supplies its own WebView for a popup (see
AwContents.supplyContentsForPopup).

Anyway, even with this approach, we had to use an additional flag. The
difference with the current approach
is only in a way when the flag is being set.

> 
> > So I resorted to catching DidStartProvisionalLoadForFrame, which seems to
work
> > reliably, as it's a synchronous callback.
> 
> Hmm, I suppose this will work, though it felt awkward.  Let me take another
look
> at the code after lunch to see if there's any ways to make it cleaner.

[Charlie Reis, 2015-03-03 22:21:01]

On 2015/03/03 21:21:14, mnaganov (cr) wrote:
> You are right -- these notifications are indeed synchronous. I must have mixed
> them up with something else,
> thanks for pointing this out!
> 
> Nevertheless, my attempt to use the pending entry which looked as below wasn't
> working reliably for the popup test:
> 
> class AwWebContentsDelegateAdapter ... {
>   private boolean mHadPendingEntry = false;
>   ...
>   public void navigationStateChanged(int flags) {
>      if ((flags & InvalidateTypes.URL) != 0) {
>        if (!mHadPendingEntry && ...getPendingEntry() != null) {
>          mHadPendingEntry = true;
>        }
>        if (mHadPendingEntry && mAwContents.hasAccessedInitialDocument()) {
>          ...
>          // synthesize callbacks
>          ...
>        }
>     }
>   }
>   ...
> }
> 
> I'm not actually sure what was the underlying reason (at that point I
> incorrectly assumed that this is because
> the notifications are delivered asynchronously), but it surely didn't work as
> expected in a reliable fashion.
> Now I'm suspecting that this is because on the Java side AwContents swaps
native
> peers of Java classes when
> the client supplies its own WebView for a popup (see
> AwContents.supplyContentsForPopup).
> 
> Anyway, even with this approach, we had to use an additional flag. The
> difference with the current approach
> is only in a way when the flag is being set.

Sure, I'm not terribly concerned about the difference between watching for
NavigationStateChanged vs DidStartProvisionalLoad.

It would be nice if we didn't need this extra step (to make it easier to reason
that the fix is still correct), but I understand the need to avoid unnecessary
notifications when those are somewhat disruptive.

LGTM with one question and a minor style nit (not that I'm a Java reviewer).

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:913:
mDidAttemptLoad = true;
What about window.open() (with no arguments)?  That creates a new window without
navigating it, so that it stays on the initial empty document.

(Maybe that doesn't go through this case?  Or maybe it's safe to treat that as
having a load anyway?)

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:16:
private boolean mDidStartProvisionalLoadForFrameFired = false;
nit: Did...Fired sounds strange to me, but I'll defer if it's following an
existing naming convention.  Otherwise, something like
mHasStartedAnyProvisionalLoad might be easier to read.

(Same for the method name.)

[mnaganov (inactive), 2015-03-04 10:30:29]

Thanks, Charlie!

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:913:
mDidAttemptLoad = true;
On 2015/03/03 22:21:00, Charlie Reis wrote:
> What about window.open() (with no arguments)?  That creates a new window
without
> navigating it, so that it stays on the initial empty document.
> 
> (Maybe that doesn't go through this case?  Or maybe it's safe to treat that as
> having a load anyway?)

I specifically checked this case, and for WebView of the popup it delivers
callbacks indicating loading of "about:blank" page, so it doesn't actually
matter, whether anything was passed to 'window.open'. Maybe this was done for
compatibility with how old versions of WebView behave, not sure.

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/970883002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:16:
private boolean mDidStartProvisionalLoadForFrameFired = false;
On 2015/03/03 22:21:01, Charlie Reis wrote:
> nit: Did...Fired sounds strange to me, but I'll defer if it's following an
> existing naming convention.  Otherwise, something like
> mHasStartedAnyProvisionalLoad might be easier to read.
> 
> (Same for the method name.)

Yeah. I was so much tired battling with the code that didn't pay much attention
to choosing a good name. Changed to the name you suggested.

[mnaganov (inactive), 2015-03-04 10:34:53]

mnaganov@chromium.org changed reviewers:
+ davidben@chromium.org, tedchoc@chromium.org

[mnaganov (inactive), 2015-03-04 10:36:26]

davidben@, tedchoc@: TBR'ing you because nothing has changed since the last
attempt in the code that you already approved, and we have time constraints for
landing this patch.

[mnaganov (inactive), 2015-03-04 10:36:36]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-03-04 10:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
torne@chromium.org
Link to the patchset: https://codereview.chromium.org/970883002/#ps20001 (title:
"Comments addressed")

[commit-bot: I haz the power, 2015-03-04 10:36:55]

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

[commit-bot: I haz the power, 2015-03-04 12:24:05]

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

[commit-bot: I haz the power, 2015-03-04 12:24:06]

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

[mnaganov (inactive), 2015-03-04 12:35:56]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-03-04 12:35:57]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
torne@chromium.org
Link to the patchset: https://codereview.chromium.org/970883002/#ps40001 (title:
"Fixed findbugs warning")

[commit-bot: I haz the power, 2015-03-04 12:36:59]

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

[commit-bot: I haz the power, 2015-03-04 13:42:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-04 13:42:50]

Patchset 3 (id:??) landed as
https://crrev.com/b2803fd408c3709889645061fad42d3371024b93
Cr-Commit-Position: refs/heads/master@{#319061}