Closed Bug 1278581 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect, P3)

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Platform:
All
Android
Type:
defect

Tracking

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

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- wontfix
fennec + ---
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: sebastian, Assigned: JanH)

References

Details

(Keywords: regression)

Attachments

(7 files, 4 obsolete files)

Add 'millisSinceLastUserInput' to nsIDOMWindowUtils
(deleted), patch
smaug
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Show neterror page in Fennec if you enter a bad URI scheme into urlbar
(deleted), patch
mcomella
: review+
Details | Diff | Splinter Review
Screenshot_2016-10-03-13-41-34.png
(deleted), image/png
Details
Bug 1278581 - Part 1 - Distinguish between no-last input and very recent user input.
(deleted), text/x-review-board-request
snorp
: review+
Details
Bug 1278581 - Part 2 - Ensure a stray key-up event doesn't end up in Gecko after committing the URL bar input.
(deleted), text/x-review-board-request
jchen
: review+
Details
Bug 1278581 - Part 3 - Don't load neterror page manually when protocol couldn't be handled.
(deleted), text/x-review-board-request
snorp
: review+
Details
Bug 1278581 - Part 4 - Ensure the progress bar doesn't get stuck after displaying the error page.
(deleted), 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.
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").
status-firefox47: --- → unaffected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
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.
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+
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?
https://hg.mozilla.org/mozilla-central/rev/41dd7c0d25eb https://hg.mozilla.org/mozilla-central/rev/a13cbb3da401
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
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+
Attached image Screenshot_2016-10-03-13-41-34.png (deleted) —
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)
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?
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.
status-firefox50: affected → wontfix
status-firefox51: affected → fix-optional
status-firefox52: affected → fix-optional
Flags: needinfo?(snorp)
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: ? → +
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".
Blocks: 1412862
[triage] non-critical.
Assignee: snorp → nobody
Priority: P2 → P3
(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.
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: nobody → jh+bugzilla
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)
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.
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.
Attachment #8961537 - Flags: review?(michael.l.comella)
Attachment #8961191 - Flags: review?(michael.l.comella)
Attachment #8961191 - Flags: review?(esawin)
Attachment #8961537 - Flags: review?(esawin)
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 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+
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.
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
status-firefox61: fixed → verified
Regressions: 1252310
Regressions: 1551458
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: