Nothing happens after entering URL with unknown protocol like foo://bar

VERIFIED FIXED in Firefox 61

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
11 days ago

People

(Reporter: sebastian, Assigned: JanH)

Tracking

(Blocks 1 bug, {regression})

unspecified
Firefox 61
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 wontfix, firefox49 wontfix, fennec+, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 unaffected, firefox59 wontfix, firefox60 wontfix, firefox61 verified)

Details

Attachments

(7 attachments, 4 obsolete attachments)

2.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.28 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
21.89 KB, image/png
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
esawin
: review+
Details
STR:
* Enter URL foo://bar

Expected result:
Desktop Firefox performs a search.

Actual result:
Nothing happens, no search, no error.
See Also: → 1278616
I debugged this a bit and it seems like we do not receive a single event in Tab.java after "submitting" the new URL.
Is this a regression? It seems like we should be following the same logic as desktop Firefox.
tracking-fennec: --- → ?
Yeah, it seems like FF47 is unaffected and shows an error page ("The address wasn't understood").
We should track this for 48. Sebastian, can you investigate?
Assignee: nobody → s.kaspari
tracking-fennec: ? → 48+
Flags: needinfo?(s.kaspari)
Yep!
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Tested using device: LG G4 (Android 5.1);

Regression window:
Last good build: 2016-03-29
First bad build: 2016-03-30

pushlog:https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8&tochange=d5d53a3b4e50b94cdf85d20690526e5a00d5b63e
(In reply to Sorina Florean [:sorina] from comment #6)
> Tested using device: LG G4 (Android 5.1);
> 
> Regression window:
> Last good build: 2016-03-29
> First bad build: 2016-03-30
> 
> pushlog:https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8&tochange=d5d5
> 3a3b4e50b94cdf85d20690526e5a00d5b63e

Thanks! That was helpful.

It seems like this patch from bug 1258605 is causing this:
https://hg.mozilla.org/mozilla-central/rev/971ee95dade0

This seems to be an intentional change (at least for clicked links). Maybe we should decide between clicked and entered URLs here (if possible).
(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> (In reply to Sorina Florean [:sorina] from comment #6)
> > Tested using device: LG G4 (Android 5.1);
> > 
> > Regression window:
> > Last good build: 2016-03-29
> > First bad build: 2016-03-30
> > 
> > pushlog:https://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8&tochange=d5d5
> > 3a3b4e50b94cdf85d20690526e5a00d5b63e
> 
> Thanks! That was helpful.
> 
> It seems like this patch from bug 1258605 is causing this:
> https://hg.mozilla.org/mozilla-central/rev/971ee95dade0

Maybe we should consider backing out that change if we can't find a good solution here.

> This seems to be an intentional change (at least for clicked links). Maybe
> we should decide between clicked and entered URLs here (if possible).

Yes, looks like an oversight (this URL loading logic is tricky), but to solve the exact problem in bug 1258605 I agree we could try to apply that to just link clicks.
Sebastian, in comment 7, correctly showed the code change that caused this regression.

The entry point (from the toolbar) is at [1]. It looks like we pass a "userEntered" flag over to js, but we don't pass it as a parameter to browser.js' loadURI [2].

To fix this bug as Margaret suggests in comment 8, we have to:
 * Pass the userEntered flag into browser.js' loadURI (and similar methods)
 * Pass the flag through until the `Intent:OpenNoHandler` message is sent – and pass it with that message as well. This message is sent to call the code Sebastian mentioned [3].

[1]: https://dxr.mozilla.org/mozilla-central/rev/e45890951ce77c3df05575bd54072b9f300d77b0/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2350
[2]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile%2Fandroid+userEntered&=mozilla-central
[3]: https://dxr.mozilla.org/mozilla-central/rev/e45890951ce77c3df05575bd54072b9f300d77b0/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#405
(In reply to Michael Comella (:mcomella) from comment #9)
> To fix this bug as Margaret suggests in comment 8, we have to:
>  ...

This gets into the Gecko url loading process so it'd probably be more efficient for someone on platform to take a look at this.

Updated

3 years ago
Flags: needinfo?(snorp)
Re-nominating so that a platform assignee can be found in triage (See comment 9 and comment 10).
Assignee: s.kaspari → nobody
Status: ASSIGNED → NEW
tracking-fennec: 48+ → ?
Assignee: nobody → snorp
tracking-fennec: ? → 48+
Flags: needinfo?(snorp)
Priority: -- → P2
tracking-fennec: 48+ → 50+
Hi Snorp, this bug is marked as tracking fennec 50+. Is there a fix in the works? I'd be happy to take an uplift in Beta50.
Flags: needinfo?(snorp)
Whoops, guard against comparing a null TimeStamp in this version
Attachment #8793774 - Flags: review?(bugs)
Attachment #8793596 - Attachment is obsolete: true
Attachment #8793596 - Flags: review?(bugs)
Hmmm. Apparently the timeout really is supposed to be to avoid returning 'true' for long-running event handlers. I guess I need to do something else here.
Ugh, this seems difficult to solve correctly. There is not really an equivalent to the 'userEntered' flag in platform code (nsIDocShell, etc), but we seem to have the opposite flag via LOAD_LINK (used for loads that come from clicking a link). The problem is that this does not get passed on to the nsExtProtocolChannel at all, which is what ends up invoking nsIExternalProtocolService::LoadURI() and doing all of the actual work. I think the only decent way to solve this may be to check in nsDocShell if the created channel is nsExtProtocolChannel and bail out (show scheme error page) if it is not LOAD_LINK. Not sure.
smaug, do you have an opinion on this? The frontend folks want to have different behavior for the external URI handler based on whether you are clicking a link or entering from address bar. It seems like the best way to do this may be hacking the docshell to do different stuff based on load type. Ugh.
Flags: needinfo?(bugs)
Attachment #8793595 - Attachment is obsolete: true
Attachment #8793597 - Attachment is obsolete: true
Attachment #8793774 - Attachment is obsolete: true
Comment on attachment 8795307 [details] [diff] [review]
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils

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

Don't forget to update the docs!

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDOMWindowUtils
Comment on attachment 8795308 [details] [diff] [review]
Show neterror page in Fennec if you enter a bad URI scheme into urlbar

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

lgtm, assuming `millisSinceLastUserInput` does describe keyboard input and not link clicks.

::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
@@ +525,5 @@
>  
>          }  else {
> +            // We return the error page here, but it will only be shown if we think the load did
> +            // not come from clicking a link. Chrome does not show error pages in that case, and
> +            // many websites have catered to this behavior. For example, the site might set a timeout and load a play 

nit: ws end of line

::: mobile/android/components/ContentDispatchChooser.js
@@ +70,5 @@
> +        // 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;

nit: Does it only represent keyboard input rather than a link click? If so, perhaps the name could be more descriptive?
Attachment #8795308 - Flags: review?(s.kaspari) → review+
(In reply to Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] from comment #24)
> nit: Does it only represent keyboard input rather than a link click? If so,
> perhaps the name could be more descriptive?

It should refer to any input, keyboard or mouse, though I am not sure if our IME stuff actually counts as user input. The naming is consistent with the other terminology in EventStateManager and nsIDOMWindowUtils.
Comment on attachment 8795307 [details] [diff] [review]
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils

># HG changeset patch
># User James Willcox <snorp@snorp.net>
>
>Bug 1278581 - Add 'millisSinceLastUserInput' to nsIDOMWindowUtils r=smaug
>
>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>index 663c5c9..362ce80 100644
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -3495,16 +3495,29 @@ NS_IMETHODIMP
> nsDOMWindowUtils::GetIsHandlingUserInput(bool* aHandlingUserInput)
> {
>   *aHandlingUserInput = EventStateManager::IsHandlingUserInput();
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsDOMWindowUtils::GetMillisSinceLastUserInput(int* aMillisSinceLastUserInput)
int32_t*, but, see the comment below

>+{
>+  TimeStamp lastInput = EventStateManager::LatestUserInputStart();
>+  if (lastInput.IsNull()) {
>+    *aMillisSinceLastUserInput = 0;
Hmm, so if there has been < 1ms since the last user input, we can't differentiate no-last input vs. very recent input.
So, perhaps make this method to return -1 when we don't know the answer.

>+    return NS_OK;
>+  }
>+
>+  *aMillisSinceLastUserInput = (TimeStamp::Now() - lastInput).ToMilliseconds();
Doesn't this cause some warning, since you're doing implicit double -> int conversion?
You could just return double from the method. double in the idl.

>   /**
>+   * Returns milliseconds elapsed since last user input was started
>+   *
>+   * This relies on EventStateManager::LatestUserInputStart()
>+   */
>+  readonly attribute long millisSinceLastUserInput;
So, s/long/double/ here

with those, r+
Flags: needinfo?(bugs)
Attachment #8795307 - Flags: review?(bugs) → review+

Comment 27

3 years ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41dd7c0d25eb
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13cbb3da401
Show neterror page if you enter an unknown URI scheme into urlbar on Android r=sebastian
Comment on attachment 8795307 [details] [diff] [review]
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils

Approval Request Comment
[Feature/regressing bug #]: Bug 1258605 
[User impact if declined]: We won't show an error page if you enter foo://bar into the address bar
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: Pretty low, but could possibly effect other instances where we encounter an unknown URI scheme.
[String/UUID change made/needed]: None

We'd need both patches on this bug
Attachment #8795307 - Flags: approval-mozilla-beta?
Attachment #8795307 - Flags: approval-mozilla-aurora?

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41dd7c0d25eb
https://hg.mozilla.org/mozilla-central/rev/a13cbb3da401
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8795307 [details] [diff] [review]
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils

Let's stabilize on Aurora51 for a few days and uplift in time for 50.0b4 on Monday.
Attachment #8795307 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested using:
Device: Moto X (Android 4.4.4)
Build: Firefox for Android 52.0a1 (2016-10-02)

With a clean profile, typing foo://bar into the URL Bar will display a blank page.
Closing that tab and load foo://bar in another tab, will display "The address wasn't understood. try again"
Snorp, I'd think both pages comment 36 should display, "The address wasn't understood." Is the current behavior expected?
Flags: needinfo?(snorp)
Yes, it should've displayed the error page both times. I can't repro that here. Fluke? Do you get the blank page on the first attempt every time?
Flags: needinfo?(snorp)
Flags: needinfo?(teodora.vermesan)

Comment 39

3 years ago
I am able to reproduce it following Teodora's steps in comment #36:
1. Have a clean profile
2. Load first foo://bar from the onboarding screen

Notes: 
- I saw that if u open it when the top sites/ pined tabs are displayed  no action will be triggered. 
- see video: https://youtu.be/MAaApUBkRO4 

Let us know how we can help more here.
Flags: needinfo?(teodora.vermesan) → needinfo?(snorp)
Interesting. I'll give it another look.
Flags: needinfo?(snorp)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8795307 [details] [diff] [review]
Add 'millisSinceLastUserInput' to nsIDOMWindowUtils

Clearing the beta flag, please renom the updated fix (assuming there will be one as the bug was reopened)
Attachment #8795307 - Flags: approval-mozilla-beta?

Updated

3 years ago
QA Contact: ioana.chiorean
Are we still going to try to fix this in 50?
Flags: needinfo?(snorp)
I still can't repro, and it seems to be fixed in most cases at least.
I can reproduce this now, so probably need to look at it once again.
tracking-fennec: 50+ → 53+
tracking-fennec: 53+ → +
This looks like this accidentally fell off so reflagging for triage. 

fwiw, I sometimes hit this when when filtering google searches on specific sites, e.g. "site:reddit.com lol". Work-around: switch the parameters, e.g. "lol site:reddit.com"
tracking-fennec: + → ?
There is no good way to solve this currently. I think we should revisit it, but not enough people to spend time on it right now.
tracking-fennec: ? → +
(Assignee)

Comment 47

2 years ago
It seems like Gecko is receiving input events (or at least the Enter key) even though the focus is on the (Java) URL bar, so the code then naturally classifies this as a link click since millisSinceLastUserInput is << 1000.
The only way I can currently get about:neterror to appear is by using "Paste & Go".
(Assignee)

Updated

2 years ago
Blocks: 1412862
[triage] non-critical.
Assignee: snorp → nobody
Priority: P2 → P3
(Assignee)

Comment 49

a year ago
(In reply to Jan Henning [:JanH] from comment #47)
> It seems like Gecko is receiving input events (or at least the Enter key)
> even though the focus is on the (Java) URL bar, so the code then naturally
> classifies this as a link click since millisSinceLastUserInput is << 1000.

I've put breakpoints on all the onKey... methods in GeckoView - nothing out of the ordinary happens when typing in the URL bar, but when I submit my input, for some reason the key-up event for the Enter key ends up being sent to the GeckoView and subsequently to Gecko.
(Assignee)

Comment 50

a year ago
More precisely, we're committing the URL bar input when receiving the key-down event for the Enter key [1]. This in turn calls BrowserApp's commitEditingMode(), which ends up hiding the URL bar input view, all while still within the key-down event handling.
This means that by the time the key-up event arrives, the focus has already moved on to the GeckoView.

[1] https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java#590,594,600,620-621,626
[2] https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2684
(Assignee)

Updated

a year ago
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8961190 [details]
Bug 1278581 - Part 1 - Distinguish between no-last input and very recent user input.

https://reviewboard.mozilla.org/r/229960/#review235776

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1658
(Diff revision 1)
>     */
>    readonly attribute boolean isHandlingUserInput;
>  
>    /**
> -   * Returns milliseconds elapsed since last user input was started
> +   * Returns milliseconds elapsed since last user input was started.
> +   * Returns -1 if there wasn't any previous user input.

This would be a surprising change for consumers of this function, but appears that Fennec is the only one.
Attachment #8961190 - Flags: review?(snorp) → review+
Comment on attachment 8961192 [details]
Bug 1278581 - Part 3 - Don't load neterror page manually when protocol couldn't be handled.

https://reviewboard.mozilla.org/r/229964/#review235778
Attachment #8961192 - Flags: review?(snorp) → review+
Attachment #8961191 - Flags: review?(snorp) → review?(michael.l.comella)
(Assignee)

Comment 56

a year ago
Hmm, with part 3 it seems that when opening a link in a new tab, we might still be suffering some variety of bug 976426.
(Assignee)

Comment 57

a year ago
mozreview-review-reply
Comment on attachment 8961190 [details]
Bug 1278581 - Part 1 - Distinguish between no-last input and very recent user input.

https://reviewboard.mozilla.org/r/229960/#review235776

> This would be a surprising change for consumers of this function, but appears that Fennec is the only one.

True, but I did check for any other in-tree consumers and didn't find any.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8961537 - Flags: review?(michael.l.comella)
(Assignee)

Updated

a year ago
Attachment #8961191 - Flags: review?(michael.l.comella)
(Assignee)

Updated

a year ago
Attachment #8961191 - Flags: review?(esawin)
Attachment #8961537 - Flags: review?(esawin)

Comment 61

a year ago
mozreview-review
Comment on attachment 8961537 [details]
Bug 1278581 - Part 4 - Ensure the progress bar doesn't get stuck after displaying the error page.

https://reviewboard.mozilla.org/r/230332/#review237100
Attachment #8961537 - Flags: review?(esawin) → review+
Jim, can you take the review for part 2?
Flags: needinfo?(nchen)

Comment 63

a year ago
mozreview-review
Comment on attachment 8961191 [details]
Bug 1278581 - Part 2 - Ensure a stray key-up event doesn't end up in Gecko after committing the URL bar input.

https://reviewboard.mozilla.org/r/229962/#review238238

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:492
(Diff revision 2)
> +        mSwallowEnterKeyUp = false;
>          // Global onKey handler. This is called if the focused UI doesn't
>          // handle the key event, and before Gecko swallows the events.
>          if (event.getAction() != KeyEvent.ACTION_DOWN) {
> +            if (shouldSwallowEnterKeyUp &&
> +                    event.getAction() == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_ENTER) {

Looking at [1], we also load URI on pressing a gamepad button. In that case we probably want to suppress that too. So we should probably suppress any key up event right after starting a URI load from key press. BTW, I think I prefer "suppress" over "swallow".

[1] https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java#620

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1293
(Diff revision 2)
>          });
>  
>          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
>              @Override
>              public void onCommit() {
>                  commitEditingMode();

`onCommit` and `commitEditingMode` should probably be renamed to indicate they are specific to key events, e.g. `onCommitByKey` and `commitEditingModeByKey`
Attachment #8961191 - Flags: review+
(Assignee)

Comment 64

a year ago
mozreview-review-reply
Comment on attachment 8961191 [details]
Bug 1278581 - Part 2 - Ensure a stray key-up event doesn't end up in Gecko after committing the URL bar input.

https://reviewboard.mozilla.org/r/229962/#review238238

> Looking at [1], we also load URI on pressing a gamepad button. In that case we probably want to suppress that too. So we should probably suppress any key up event right after starting a URI load from key press. BTW, I think I prefer "suppress" over "swallow".
> 
> [1] https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java#620

Yes, I was being a bit overly paranoid in only suppressing the correct key - but as long as we're doing this from within a key-down handler, you'right - it's okay and more flexible to just swallow the next key up regardless of which specific key it was.

> `onCommit` and `commitEditingMode` should probably be renamed to indicate they are specific to key events, e.g. `onCommitByKey` and `commitEditingModeByKey`

I had considered that, but since right now "by key" is the only way we can commit an edited URL, I didn't do anything specifically in that regard. But you're right, in principle this could change in the future, so it's better to drop some additional hints in that regard.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 72

a year ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/02ba9ffbf52f
Part 1 - Distinguish between no-last input and very recent user input. r=snorp
https://hg.mozilla.org/integration/autoland/rev/b76141a4391b
Part 2 - Ensure a stray key-up event doesn't end up in Gecko after committing the URL bar input. r=jchen
https://hg.mozilla.org/integration/autoland/rev/f9600623bcc9
Part 3 - Don't load neterror page manually when protocol couldn't be handled. r=snorp
https://hg.mozilla.org/integration/autoland/rev/cb9e04f4c32d
Part 4 - Ensure the progress bar doesn't get stuck after displaying the error page. r=esawin
Flags: needinfo?(nchen)
Verified as fixed on latest Nightly build (2018-04-04). "The address wasn't understood" message is displayed.
Devices: Nexus 5(Android 6.0.1) and Huawei MediaPad M2 (Android 5.1.1).
Status: RESOLVED → VERIFIED
(Assignee)

Updated

11 days ago
Regressions: 1252310
You need to log in before you can comment on or make changes to this bug.