1478171 - Links with redirects do not come through onLoadRequest

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Emily Kager [:ekager]]

When investigating this issue (https://github.com/mozilla-mobile/focus-android/issues/2911) I found that in WV's shouldOverrideUrlLoading method, the custom scheme comes through and triggers our handling of external content through an Intent. If you copy + paste the custom scheme (for example: reddit://reddit/?link_click_id=550437242878965404), it works in GV, but the custom scheme does not come through onLoadRequest by itself when clicking on the open in App click on Reddit. 

STR:
1. Go to reddit.com
2. Click Open in App

Expected: 
Launches Reddit app

Actual:
Nothing happens

Comment 1

[Chris Peterson [:cpeterson]]

P1 because this would be a functional regression when switching from WV to GV.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1478171-1.0-Forward-channel-redirect-to-nsILoadU.patch

The issue here is not specifically tied to custom schemas or intent URLs, we generally don't forward page load redirects to nsILoadURIDelegate to allow app-specific handling.

smaug, is this the right place to intercept a channel redirect chain? Do we need to abort the redirection somehow differently to avoid leaving channels in a bad state?

Is this a good idea generally or do you see a better way of allowing GeckoView apps to override page load behavior when triggered through a redirect?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Let's file a new bug for the intent URI filtering? (Probably not blocking)

This bug should address redirects.

Comment 5

[Chris Peterson [:cpeterson]]

We decided this bug does not need to block Focus+GV.

Comment 6

[Eugen Sawin [:esawin]]

Reassigning to Dylan while I'm on leave.

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Attachment: Allow GV apps to intercept redirects in nsILoadURIDelegate

This updates Eugen's patch to work with the async nsILoadURIDelegate.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

># HG changeset patch
># User Dylan Roeh <[e-mail]>
># Date 1534873959 18000
>#      Tue Aug 21 12:52:39 2018 -0500
># Node ID 1dfd595e1d3ddb387e5ee797b85b34aeac06f107
># Parent  130bb81b5f4f9c98ba138ab5593148537dd77b57
>Bug 1478171 - [1.0] Forward channel redirect to nsILoadURIDelegate to allow external handling. r=smaug,snorp
>
>diff --git a/netwerk/base/nsIChannelEventSink.idl b/netwerk/base/nsIChannelEventSink.idl
>--- a/netwerk/base/nsIChannelEventSink.idl
>+++ b/netwerk/base/nsIChannelEventSink.idl
>@@ -48,16 +48,23 @@ interface nsIChannelEventSink : nsISuppo
>     /**
>      * This is a special-cased redirect coming from hitting HSTS upgrade
>      * redirect from http to https only.  In some cases this type of redirect
>      * may be considered as safe despite not being the-same-origin redirect.
>      */
>     const unsigned long REDIRECT_STS_UPGRADE = 1 << 3;
> 
>     /**
>+     * This redirect has already been presented to the GeckoView app delegate
>+     * for possible handling; if this flag is set we may safely skip checking
>+     * the nsILoadURIDelegate to see if the app wants to handle the request.
This kind of API documentation shouldn't talk about one specific user. So drop talk about GeckoView.


>+class LoadURIDelegateRedirectHandler final : public mozilla::dom::PromiseNativeHandler
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_CLASS(LoadURIDelegateRedirectHandler)
>+
>+  LoadURIDelegateRedirectHandler(nsDocLoader *aDocLoader,
>+                                 nsIChannel *aOldChannel,
>+                                 nsIChannel *aNewChannel,
>+                                 uint32_t aFlags,
>+                                 nsIAsyncVerifyRedirectCallback *cb)
New code should use Mozilla coding style. So, * goes with type and s/cb/aCb/


I hope there will be some tests for this stuff on GeckoView side, or somewhere.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

Review of attachment 9002857 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok with possibly the NS_OK being switched.

::: uriloader/base/nsDocLoader.cpp
@@ +1447,5 @@
> +  ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
> +  {
> +    if (!aValue.isBoolean() || (aValue.isBoolean() && !aValue.toBoolean())) {
> +      // This redirect wasn't handled, let Gecko deal with it.
> +      mFlags |= nsIChannelEventSink::REDIRECT_DELEGATES_CHECKED;

Consider factoring out these few lines since you do the same thing in RejectedCallback()

@@ +1452,5 @@
> +      mDocLoader->AsyncOnChannelRedirect(mOldChannel, mNewChannel, mFlags,
> +                                         mCallback);
> +    } else {
> +      // The app handled the redirect, notify the callback
> +      mCallback->OnRedirectVerifyCallback(NS_OK);

It looks like we should pass NS_ERROR_ABORT or similar here, IMHO, since we effectively vetoed the redirect.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cfd6113ca7
[1.0] Forward channel redirect to nsILoadURIDelegate to allow external handling. r=smaug,snorp

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/94cfd6113ca7

Comment 12

[Stefan Deiac]

Verified as fixed on Nightly 63.0a1 (08-23-2018).
Devices: Motorola Nexus 6 (Android 7.1.1), Nokia 6 (Android 7.1.1) and OnePlus 5T (Android 8.1.0)

Comment 13

[Chris Peterson [:cpeterson]]

Dylan, do we need to uplift this fix to the GECKOVIEW_62_RELBRANCH release branch for the Focus Beta? Emily suggested that we might want to uplift it since this would be a functional regression from Focus+WebView. If so, can you please request uplift for approval-mozilla-geckoview62?

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: N/A

User impact if declined: Some breakage in Focus w/GV due to redirects not being passed to onLoadRequest

Fix Landed on Version: 63

Risk to taking this patch (and alternatives if risky): Low-to-moderate; we make significant changes to how redirects work for GeckoView. So far testing has shown no issues, and any fallout should impact only GeckoView apps.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah we want this in GV62.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

GV change, needed for Focus release

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-release/rev/99aa10a241f65eac26672b56f94a0aabaeeed8c7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from GECKOVIEW_62_RELBRANCH for causing bug 1489257.
https://hg.mozilla.org/releases/mozilla-release/rev/4cdeecd31350b13f8741202dc06d87d8bc3d84db

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Backed out in bug 1489257

Comment 20

[Chris Peterson [:cpeterson]]

63=wontfix because launching app links (like Reddit's) is a low priority for Focus.

Comment 21

[Dylan Roeh (:droeh) (he/him)]

Attachment: Expose redirects to the LoadRequestDelegate

Comment 22

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

Forgot to flag smaug for review. This just revives Eugen's old patch; product apparently considers this important, so we'll go this route for the moment and update if necessary when we know for sure what we're doing about bug 1441059.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

Review of attachment 9021584 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok to me...

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

> {
>+  if (aFlags &
>+      (nsIChannelEventSink::REDIRECT_TEMPORARY |
>+       nsIChannelEventSink::REDIRECT_PERMANENT)) {
>+    nsCOMPtr<nsIDocShell> docShell =
>+      do_QueryInterface(static_cast<nsIRequestObserver*>(this));
>+
>+    nsCOMPtr<nsILoadURIDelegate> delegate;
>+    docShell->GetLoadURIDelegate(getter_AddRefs(delegate));
please null check docshell. Not all DocLoaders are DocShells.



>+
>+    nsCOMPtr<nsIURI> newURI;
>+    aNewChannel->GetURI(getter_AddRefs(newURI));
perhaps do this only if there is delegate.



so perhaps
nsCOMPtr<nsILoadURIDelegate> delegate;
nsCOMPtr<nsIURI> newURI;
if (docshell) {
  docShell->GetLoadURIDelegate(getter_AddRefs(delegate));
  if (delegate) {
    aNewChannel->GetURI(getter_AddRefs(newURI));
  }
}

>+
>+    if (newURI && delegate) {
and then this could have just check for newURI

>+      const int where = nsIBrowserDOMWindow::OPEN_CURRENTWINDOW;
>+      bool loadURIHandled = false;
>+      nsresult rv = delegate->LoadURI(newURI, where, /* flags */ 0,
>+                                      /* triggering principal */ nullptr,
>+                                      &loadURIHandled);
>+      if (NS_SUCCEEDED(rv) && loadURIHandled) {
>+        cb->OnRedirectVerifyCallback(NS_OK);
>+        return NS_OK;
>+      }
>+    }
>+  }
>+
>   if (aOldChannel)
>   {
>     nsLoadFlags loadFlags = 0;
>     int32_t stateFlags = nsIWebProgressListener::STATE_REDIRECTING |
>                          nsIWebProgressListener::STATE_IS_REQUEST;
> 
>     aOldChannel->GetLoadFlags(&loadFlags);
>     // If the document channel is being redirected, then indicate that the

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be2ca9d4fb4c
[1.0] Forward channel redirect to nsILoadURIDelegate to allow external handling. r=snorp,smaug

Comment 26

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Redirects will not be able to be handled by onLoadRequest in GeckoView apps -- as a result, links that should be handled by the app (eg, intent:// URIs) will not be handled correctly if they are the result of a redirect.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): I'd say somewhere between low and medium here: it only affects GeckoView, but does make a non-trivial change to how redirects are handled in GV. GV unit tests seem to suggest things are fine.

String changes made/needed: None.

Comment 27

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/be2ca9d4fb4c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

[Triage Comment]
Improves redirect handling in GeckoView, approved for 64.0b7.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4c7bbc4c6fbc

Comment 30

[Chris Peterson [:cpeterson]]

status-geckoview64=fixed