Links with redirects do not come through onLoadRequest

RESOLVED FIXED in Firefox 64

Status

defect
P1
normal
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: ekager, Assigned: droeh)

Tracking

unspecified
mozilla65
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(geckoview62 wontfix, geckoview64 fixed, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed, firefox65 fixed)

Details

(Whiteboard: [geckoview:klar:p2])

Attachments

(2 attachments, 1 obsolete attachment)

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
P1 because this would be a functional regression when switching from WV to GV.
Priority: -- → P1
Whiteboard: [geckoview:klar:p1]
Assignee: nobody → esawin
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?
Attachment #8996808 - Flags: feedback?(bugs)
Whiteboard: [geckoview:klar:p1] → [geckoview:klar:p2]
Let's file a new bug for the intent URI filtering? (Probably not blocking)

This bug should address redirects.
Flags: needinfo?(esawin)
Whiteboard: [geckoview:klar:p2] → [geckoview:klar:p1]
We decided this bug does not need to block Focus+GV.
Summary: Links with custom schemes do not come through onLoadRequest → Links with redirects do not come through onLoadRequest
Whiteboard: [geckoview:klar:p1] → [geckoview:klar:p2]
Depends on: 1480757
Flags: needinfo?(esawin)
Reassigning to Dylan while I'm on leave.
Assignee: esawin → droeh
This updates Eugen's patch to work with the async nsILoadURIDelegate.
Attachment #8996808 - Attachment is obsolete: true
Attachment #8996808 - Flags: feedback?(bugs)
Attachment #9002857 - Flags: review?(snorp)
Attachment #9002857 - Flags: review?(bugs)
Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

># HG changeset patch
># User Dylan Roeh <droeh@mozilla.com>
># 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.
Attachment #9002857 - Flags: review?(bugs) → review+
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.
Attachment #9002857 - Flags: review?(snorp) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cfd6113ca7
[1.0] Forward channel redirect to nsILoadURIDelegate to allow external handling. r=smaug,snorp
https://hg.mozilla.org/mozilla-central/rev/94cfd6113ca7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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)
Status: RESOLVED → VERIFIED
Blocks: 1486525
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?
Flags: needinfo?(droeh)
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.
Flags: needinfo?(droeh)
Attachment #9002857 - Flags: approval-mozilla-geckoview62?
Comment on attachment 9002857 [details] [diff] [review]
Allow GV apps to intercept redirects in nsILoadURIDelegate

GV change, needed for Focus release
Attachment #9002857 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62+
Attachment #9002857 - Flags: approval-mozilla-geckoview62+ → approval-mozilla-geckoview62-
Backed out in bug 1489257
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 1487542
63=wontfix because launching app links (like Reddit's) is a low priority for Focus.
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.
Attachment #9021584 - Flags: review?(bugs)
Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

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

Seems ok to me...
Attachment #9021584 - Flags: review?(snorp) → review+
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
Attachment #9021584 - Flags: review?(bugs) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be2ca9d4fb4c
[1.0] Forward channel redirect to nsILoadURIDelegate to allow external handling. r=snorp,smaug
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.
Attachment #9021584 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/be2ca9d4fb4c
Status: REOPENED → RESOLVED
Closed: Last year10 months ago
Resolution: --- → FIXED
Target Milestone: Firefox 63 → Firefox 65
Flags: qe-verify-
Flags: in-testsuite+
Comment on attachment 9021584 [details] [diff] [review]
Expose redirects to the LoadRequestDelegate

[Triage Comment]
Improves redirect handling in GeckoView, approved for 64.0b7.
Attachment #9021584 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-geckoview64=fixed
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.