New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 642848 link

Starred by 6 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

preventDefault on willSubmitForm inhibits tab-to-search engine creation

Project Member Reported by pkasting@chromium.org, Aug 31 2016

Issue description

Chrome Version       : 54.0.2840.6
OS Version: 10.0

On a brand-new profile, visit amazon.com.  Use the search box in the page.  No tab-to-search entry for Amazon gets added.
 
Confirmed.  I tried several web sites on a clean profile and could not get a tab-to-search entry created.

Labels: Needs-Bisect
I'm sure this used to work.  Can we get a bisect please?

More detailed repro instructions:
1. on a brand-new profile, go to amazon.com.   Make sure it fully loads.
2. use the search box on the page to search for something.  Make sure the result page fully loads.
3. wait ten seconds.
4. use the omnibox and type amazon.com and press tab.

Correct behavior:
5. the omnibox should show a "Search amazon.com:" box.
6. also, in this case, you should be able to go to about:settings "Manage search engines" section and see amazon.com listed in the "other search engines" category.

Incorrect behavior:
5. pressing tab scrolls through the omnibox dropdown.
6. also, in this case, if you go to about:settings "Manage search engines" section, amazon.com will not be listed anywhere.

Comment 3 by ajha@chromium.org, Sep 30 2016

Cc: ajha@chromium.org
Labels: -Needs-Bisect M-55
Able to reproduce the issue on the latest canary(55.0.2876.0) and the latest stable(53.0.2785.143) on Windows-10, Mac OS 10.11.6 and Linux Ubuntu 14.04 as per the test steps in C#2.

Same behavior is seen on amazon.com, on older chrome version: 30.0.1549.0. Attached is the screen-cast of the same.

Triaging this as Non-regression issue and for further investigation.
642848.mp4
4.8 MB View Download
Owner: pkasting@chromium.org
Status: Assigned (was: Available)
ajha@: thanks for checking!

pkasting@: how strange.  Has this been broken for that long?  It works--the search engine is auto-generated--in my current install of Safari (which, by the way, has javascript disabled).   But I am having trouble figuring out how Safari knows to create the search engine.  Using the web inspector, a search for "opensearch" comes up empty.  You're more familiar with the opensearch standard than me; can you please figure out how Safari is creating this engine correctly?

According to my read of the document
http://www.opensearch.org/Specifications/OpenSearch/1.1#Autodiscovery
, all the possible ways of notifying a browser of the existence of an opensearch ability use the word "opensearch" somewhere.  I must be missing something.)
Cc: pkasting@chromium.org
Owner: ----
I would look for "search" rather than "opensearch" -- "link rel='search'" is the canonical OSDD route I know (but I don't see that on the Amazon page I looked at).

I don't know how our autodetection works.

I would assume that something about Amazon changed in a way we no longer detect but Safari does, but it's also possible Amazon is serving Safari a different source than Chrome and they need to fix it.  Either way someone who knows anything at all about web development, i.e. can use the dev tools to look at differences between these pages, should be involved here.

Since none of the above useful knowledge areas are me, I don't think I should own this.
Status: Available (was: Assigned)
> I don't see that on the Amazon page I looked at.
Nor do I.

> it's also possible Amazon is serving Safari a different source than Chrome
That's true, but Safari can auto-generate a search engine from whatever HTML+Javascript it's being served, so we ought to be able to figure out how it does that using Safari's web inspector.

Owner: mpear...@chromium.org
Status: Assigned (was: Available)
Asked for help on the opensearch mailing list:
https://groups.google.com/d/msg/opensearch/EcT73zePg18/WZPGQLqkAwAJ

Thus far, no assistance. :-(

Cc: ashej...@chromium.org
 Issue 256751  has been merged into this issue.
Labels: -Pri-1 Pri-2
Downgrading priority because I tried other sites and some work.  For example, etsy.com does cause creation of a search engine.  Other sites such as walmart.com and target.com do.  Yet, they cause creation of a search engine on Safari.
Pretty sure one of those "does" or "do"s was meant to be negated (the latter?).
> Pretty sure one of those "does" or "do"s was meant to be negated (the latter?).
Correct.  The latter (walmart and target) do not cause creation of a search engine.
My best guess from looking at the sources of these pages is that Chromium only creates a search engine when it finds an opensearch element such as on etsy.com
<link type="application/opensearchdescription+xml" rel="search" href="/osdd.php">
but Safari creates one whenever it finds a form with role="search", no need for an opensearch document.

It would be useful if someone could look through the Safari WebKit code to confirm.
Our docs claim that we don't require an OSDD as long as you do some other stuff: https://www.chromium.org/tab-to-search

Maybe we need to expand our heuristics to detect more engines?
> Our docs claim that we don't require an OSDD as long as you do some other stuff:
> https://www.chromium.org/tab-to-search
>>>
If the site does not provide a link to an OpenSearch description document but the user submits a form, then Chrome automatically adds the site to the list of searchable sites. There are a number of restrictions with this approach though. In particular the form must generate a GET, must result in a HTTP url, and the form must not have OnSubmit script. Additionally there must be only one input element of type text, no passwords, files or text areas and all other input elements must be in their default state.
>>>

> Maybe we need to expand our heuristics to detect more engines?
Indeed.  This HTTP restriction in this day and age is preposterous.  We should allow HTTPS search engines.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSearchableFormData.cpp?l=240

That alone could explain the lack of amazon and walmart (both of which are https by default).

That doesn't explain target; maybe another heuristic is in play there.
I think we originally restricted HTTPS to forms exposing an input element to try to avoid exposing sensitive or secure data/sites as a "search engine".  I'm OK with relaxing the restrictions here as long as we think carefully through "how could this go wrong and make my bank/sensitive corporate intranet login a search engine".

I bet we could remove the POST restriction (don't we support POST properly now?), but in practice that probably fixes few real sites.
Status: Started (was: Assigned)
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0bc8993c2a2766309e91904b742936202bed2b45

commit 0bc8993c2a2766309e91904b742936202bed2b45
Author: mpearson <mpearson@chromium.org>
Date: Mon Nov 28 20:23:41 2016

Auto-Generating Search Engines - Remove HTTP Restriction

Remove the restriction that, under certain conditions, we require
the protocol of an auto-created custom search engine be HTTP.  The
condition has no connection with a need for privacy.  There is no
problem with creating custom search engines for secure sites.  In
fact, we should be encouraging more sites to be secure, not penalizing
those that do so by making it more difficult for them to create a
custom search engine.

BUG=642848
TEST=amazon.com now gets a custom search engine created

Review-Url: https://codereview.chromium.org/2505933005
Cr-Commit-Position: refs/heads/master@{#434706}

[modify] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/WebSearchableFormData.cpp
[modify] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/WebSearchableFormDataTest.cpp
[rename] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/data/search_form_http.html
[add] https://crrev.com/0bc8993c2a2766309e91904b742936202bed2b45/third_party/WebKit/Source/web/tests/data/search_form_https.html

Summary: Walmart and Target no longer auto-detected as search engine (was: Amazon no longer auto-detected as search engine)
amazon.com is now fixed.  Walmart and target are still broken.

>>>
I think we originally restricted HTTPS to forms exposing an input element to try to avoid exposing sensitive or secure data/sites as a "search engine".  I'm OK with relaxing the restrictions here as long as we think carefully through "how could this go wrong and make my bank/sensitive corporate intranet login a search engine".
>>>
I also check a few banks and bank-like places and did not see any errant custom search engines created.

Happily, I checked this in well before a branch point, so if anything weird comes up, it'll be easy to revert.

I'll morph this into a bug about the remaining bad examples.
I added debugging lines.  For walmart and target, WebSearchableFormData::WebSearchableFormData() is not called.

The two relevant places this can be called are
RenderFrameImpl::BeginNavigation()
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSearchableFormData.cpp?l=218
and
RenderFrameImpl::willSubmitForm()
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=3246

(It's also called somewhere in the context menu code too.)

The amazon.com case goes through second case.

Now I (or hopefully someone with knowledge of how form navigations should happen) need to figure out why we're not triggering a willSubmitForm() for walmart and target.
It looks like both walmart and target have Javascript event handlers for form submissions that call e.preventDefault().  Thus--I think this is a "thus"--RenderFrameImpl::willSubmitForm() is never called.
Heh, I re-checked Safari.  It appears my Safari has Javascript disabled.  (Don't remember when I set that.)  Under these conditions, I previously reported that Safari creates search engines for walmart and target.  When I have Javascript enabled, it does not, just like Chrome.

I guess Safari isn't doing anything better than Chrome here.  And I don't think we can stop sites like these from shooting themselves in the foot.  I don't see how if they call preventDefault, we can still decide to create a search engine.

I guess I'm coming around to sadly closing this as WontFix, unless someone else has something to say...
If willSubmitForm is never called, the form shouldn't be submitted at all, right?  So how does the search actually occur?
>>>
If willSubmitForm is never called, the form shouldn't be submitted at all, right?  So how does the search actually occur?
>>>
Target constructs the destination URL in Javascript and then uses window.location.assign deep in the bowels of the Javascript.

navigateSearch: function(e, t, r, i) {
    isSecure ? (e = e.charAt(0) === "/" ? e : "/" + e,
    e = e.replace("/mobile", ""),
    window.location.assign(domain + e)) : typeof i == "undefined" ? n.history.navigate(e, {
trigger: t,
replace: r
    }) : n.history.loadUrl(e)
},


The Walmart Javascript does something similar by assigning this._getWindow().location.href directly.

// _handleEmptyInput is misnamed; it gets called on all form submits.
t.prototype._handleEmptyInput = function(e) {
    e.preventDefault();
    var t = this._getSearchInput()
      , n = this._getWindow().document.querySelector("#global-search-catid")
      , r = void 0;
    if (n && (r = this._getWindow().document.querySelector("#global-search-catid").value),
    /\S/.test(t)) {
var o = "/search/?query=" + t;
r && (o += "&cat_id=" + r),
this._getWindow().location.href = o
    }
}

I wonder if we should try and detect "navigated to something that would match the destination URL of the form" as an alternative to "submitted the form".
We'd have to be careful.  I see shopping websites that have a dropdown in the form that let the users select a department to search (clothing, electronics, etc.).  These web sites also have links on the page to general category pages (e.g., browse clothing).  I can easily imagine that sometime the general browsing link goes to the same as the the search form does, simply with an empty search string.  I don't think we should create a search engine in these cases because the user didn't use the web site to search.  The form may not have registered in their mind.

I guess we should look for non-empty search strings and also maybe non-default value search strings.

Are there other things that can bite us here?

As for how, this is the only way I see.  It seems a bit heavy-handed.  On every page load, would we look over the form(s) on the web page, remember them, then wait for any sort of navigation in this renderer and attempt to match the form(s) to the destination URL.  (Right now Chrome only remembers the one relevant form on willSubmitForm and then waits for the new navigation to happen.  It knows the form is relevant.)
Rather than looking at all forms, I would only handle the specific case where a page did a preventDefault() on willSubmitForm and then navigated itself.  In other words, I would try to detect this case as a "submitted form", rather than the more aggressive "match any navigation to any form".

The "and then" is potentially tricky.  "And then" in the same synchronous callstack?  Can we detect that?
Owner: ----
Status: Available (was: Started)
Summary: Forms that Use preventDefault and Navigate Explicitly Prevent Automatic Creation of Custom Search Engine (was: Walmart and Target no longer auto-detected as search engine)
Clarifying the subject line for the remaining issue on this bug and unassigning from myself.  Hopefully someone who's an expert on Blink + Javascript interaction can look into it, answer the question ("how do we detect 'and then' navigated?"), and maybe even take this on.  Peter, can you nominate someone who knows this area?
Components: -UI>Browser>Omnibox UI>Browser>Navigation Blink>Forms>Submission
Status: Untriaged (was: Available)
Summary: preventDefault on willSubmitForm inhibits tab-to-search engine creation (was: Forms that Use preventDefault and Navigate Explicitly Prevent Automatic Creation of Custom Search Engine)
I'm clueless :(  Adding some likely areas and setting as untriaged.

For triagers, brief summary:
* Chrome detects form submission and tries to create omnibox search engine entries if applicable, to facilitate tab-to-search
* Some sites preventDefault on willSubmitForm and then manually navigate to the submission result instead (why???)
* As a result we don't create tab-to-search entries for major sites like Walmart and Target
* Thought: detect a prevented willSubmitForm followed (synchronously?) by a navigation to something like the form submission URL and trigger the same search engine detection code as if the form were actually submitted

See comments 21 onward for more detail.
Cc: abarth@chromium.org
abarth@, perhaps you comment on this?  It seems like it touches on areas you know.  See comment #29.
Status: Available (was: Untriaged)
tkent@: you converted this bug from Untriaged to Available.  Can you please comment on the feasibility of the proposal in comment #29 (the most recent content-full comment)?  I assume you must've read it when flipping the status.  Did you decide that it's reasonable / possible?

Comment 33 by tkent@chromium.org, Dec 14 2016

> you converted this bug from Untriaged to Available.

I had to do so because we shouldn't leave bugs as Untriaged :)

Anyway, My impression:

 * Detecting prevented form submission + navigation would be too complicated. I don't want to introduce such complexity just for a limited number of sites.

 * Changing willSubmitForm timing from navigation to just after 'submit' event dispatching may work.  It would introduce false detection.

@33: I think the prevention logic here may be coming from jQuery, which may mean that "limited number of sites" is inaccurate.  Not sure.

Comment 35 Deleted

Just waylaid ojan@ and adamk@ to discuss this issue.  From a web platform perspective, Ojan said that in principle it's okay to create custom search engines even if the web site sets preventDefault in willSubmitForm.  If the web site wants to prevent custom search engine creation, they should use a different way.

He also said changing this behavior--creating search engines in this condition--will require an "intent to ship" message to blink-dev.

Ojan said he has ideas on which team should own this bug.  (It's clear the omnibox team doesn't have the knowledge to muck around with either the javascript or navigation stacks.)  Ojan, can you please revise the components, tags, bug status, etc. as appropriate?  In your message, you can probably point out that the short summary of the bug is comment #29.  thanks!

Comment 37 by ojan@chromium.org, Feb 22 2017

Cc: -abarth@chromium.org dcheng@chromium.org nasko@chromium.org
I think the fixes to this would be in the navigation code. So, Nasko, I think it'd be you're team? +dcheng so he can comment on feasibility of implementation.

Regarding a different way to prevent custom search engine creation, ideally we'd give them a simple way to opt-out. FeaturePolicy could probably give us a simple, low-effort way to add a directive here.
ping nasko@ per comment #37.

Cc: vabr@chromium.org japhet@chromium.org
There's no reason why the callback can't be moved, but someone will need to do due diligence that it doesn't have any unanticipated side effects.

Researching this is technically easy: these are just callbacks from Blink to the embedder, so it's "just" a matter of moving https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?rcl=0f727a7e734fbbf7f018cda55a97bb751d8bcdac&l=1467 to be called after https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLFormElement.cpp?rcl=eee2186f0e38677b7fa0e804da2037b07ab7b6c5&l=336. The tricky part of this would making sure this doesn't break extant uses of this callback in the embedder (it looks like it's mainly used for autofill).
Labels: -M-55
Owner: japhet@chromium.org
Status: Assigned (was: Available)
japhet@, I see your name as someone who touches both these files.  Would you be a good person to own this?
Cc: -ashej...@chromium.org
Cc: -vabr@chromium.org
vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.

Sign in to add a comment