Bug 1278581 - Part 3 - Don't load neterror page manually when protocol couldn't be handled. r?snorp draft
authorJan Henning <jh+bugzilla@buttercookie.de>
Wed, 21 Mar 2018 22:47:17 +0100
changeset 775265 c1bb1f245f7578f260eeab230a479539c01c5ecc
parent 775264 99d86f385510fd7a50cba892ab2601d53c25fd9f
child 775266 a8bcb7c80429b508014a5781bb5375a19e54347d
push id104675
push usermozilla@buttercookie.de
push dateFri, 30 Mar 2018 19:09:15 +0000
reviewerssnorp
bugs1278581
milestone61.0a1
Bug 1278581 - Part 3 - Don't load neterror page manually when protocol couldn't be handled. r?snorp Our current way is somewhat convoluted - we manually create an about:neterror URL in the Android UI and then load it from the ContentDispatchChooser by simply setting window.location.href, which means that the page URL won't reflect the actual URL we were trying to load and the rest of Gecko will think that the page load succeeded (unless they're explicitly checking the URL for the presence of about:neterror). MozReview-Commit-ID: Z0LSSE6AGU
mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
mobile/android/chrome/content/browser.js
mobile/android/components/ContentDispatchChooser.js
--- a/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
@@ -58,19 +58,16 @@ public final class IntentHelper implemen
         "Intent:OpenForResult",
         "Intent:OpenNoHandler",
     };
 
     // via http://developer.android.com/distribute/tools/promote/linking.html
     private static final String MARKET_INTENT_URI_PACKAGE_PREFIX = "market://details?id=";
     private static final String EXTRA_BROWSER_FALLBACK_URL = "browser_fallback_url";
 
-    /** A partial URI to an error page - the encoded error URI should be appended before loading. */
-    private static final String UNKNOWN_PROTOCOL_URI_PREFIX = "about:neterror?e=unknownProtocolFound&u=";
-
     private static IntentHelper instance;
 
     private IntentHelper() {
         EventDispatcher.getInstance().registerGeckoThreadListener(this, GECKO_EVENTS);
         EventDispatcher.getInstance().registerUiThreadListener(this, UI_EVENTS);
     }
 
     public static IntentHelper init() {
@@ -500,37 +497,28 @@ public final class IntentHelper implemen
      *                 the uri to load if Java does not load a page
      */
     private void openNoHandler(final GeckoBundle msg, final EventCallback callback) {
         final String uri = msg.getString("uri");
         final GeckoBundle errorResponse = new GeckoBundle();
 
         if (TextUtils.isEmpty(uri)) {
             Log.w(LOGTAG, "Received empty URL - loading about:neterror");
-            errorResponse.putString("uri", getUnknownProtocolErrorPageUri(""));
             errorResponse.putBoolean("isFallback", false);
             callback.sendError(errorResponse);
             return;
         }
 
         final Intent intent;
         try {
             // TODO (bug 1173626): This will not handle android-app uris on non 5.1 devices.
             intent = Intent.parseUri(uri, 0);
         } catch (final URISyntaxException e) {
-            String errorUri;
-            try {
-                errorUri = getUnknownProtocolErrorPageUri(URLEncoder.encode(uri, "UTF-8"));
-            } catch (final UnsupportedEncodingException encodingE) {
-                errorUri = getUnknownProtocolErrorPageUri("");
-            }
-
             // Don't log the exception to prevent leaking URIs.
             Log.w(LOGTAG, "Unable to parse Intent URI - loading about:neterror");
-            errorResponse.putString("uri", errorUri);
             errorResponse.putBoolean("isFallback", false);
             callback.sendError(errorResponse);
             return;
         }
 
         // For this flow, we follow Chrome's lead:
         //   https://developer.chrome.com/multidevice/android/intents
         final String fallbackUrl = intent.getStringExtra(EXTRA_BROWSER_FALLBACK_URL);
@@ -570,17 +558,16 @@ public final class IntentHelper implemen
             // many websites have catered to this behavior. For example, the site might set a timeout and load a play
             // store url for their app if the intent link fails to load, i.e. the app is not installed.
             // These work-arounds would often end with our users seeing about:neterror instead of the intended experience.
             // While I feel showing about:neterror is a better solution for users (when not hacked around),
             // we should match the status quo for the good of our users.
             //
             // Don't log the URI to prevent leaking it.
             Log.w(LOGTAG, "Unable to open URI, maybe showing neterror");
-            errorResponse.putString("uri", getUnknownProtocolErrorPageUri(intent.getData().toString()));
             errorResponse.putBoolean("isFallback", false);
             callback.sendError(errorResponse);
         }
     }
 
     private static boolean isFallbackUrlValid(@Nullable final String fallbackUrl) {
         if (fallbackUrl == null) {
             return false;
@@ -596,26 +583,16 @@ public final class IntentHelper implemen
             }
         } catch (final URISyntaxException e) {
             // Do not include Exception to avoid leaking uris.
             Log.w(LOGTAG, "URISyntaxException parsing fallback URI");
         }
         return false;
     }
 
-    /**
-     * Returns an about:neterror uri with the unknownProtocolFound text as a parameter.
-     * @param encodedUri The encoded uri. While the page does not open correctly without specifying
-     *                   a uri parameter, it happily accepts the empty String so this argument may
-     *                   be the empty String.
-     */
-    private String getUnknownProtocolErrorPageUri(final String encodedUri) {
-        return UNKNOWN_PROTOCOL_URI_PREFIX + encodedUri;
-    }
-
     private static class ResultHandler implements ActivityResultHandler {
         private final EventCallback callback;
 
         public ResultHandler(final EventCallback callback) {
             this.callback = callback;
         }
 
         @Override
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -4563,17 +4563,18 @@ Tab.prototype = {
     }
 
     // Update the page actions URI for helper apps.
     if (BrowserApp.selectedTab == this) {
       ExternalApps.updatePageActionUri(fixedURI);
     }
 
     if ((!aRequest || Components.isSuccessCode(aRequest.status)) &&
-        !fixedURI.displaySpec.startsWith("about:neterror") && !this.isSearch) {
+        !(aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
+        !this.isSearch) {
       // If this won't end up in an error page and the user isn't searching,
       // don't retain the typed entry.
       this.userRequested = "";
     }
 
     let message = {
       type: "Content:LocationChange",
       tabID: this.id,
--- a/mobile/android/components/ContentDispatchChooser.js
+++ b/mobile/android/components/ContentDispatchChooser.js
@@ -81,17 +81,17 @@ ContentDispatchChooser.prototype =
         }
 
         // We couldn't open this. If this was from a click, it's likely that we just
         // want this to fail silently. If the user entered this on the address bar, though,
         // we want to show the neterror page.
         let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
         let millis = dwu.millisSinceLastUserInput;
         if (millis < 0 || millis >= 1000) {
-          window.location.href = data.uri;
+          window.document.docShell.displayLoadError(Cr.NS_ERROR_UNKNOWN_PROTOCOL, aURI, null);
         } else {
           this._closeBlankWindow(window);
         }
       });
     }
   },
 };