1435615 - One-Click search engine uses GET instead of POST the first time after each start

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[PilzAdam]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180131010234

Steps to reproduce:

0) Create a new profile in current nightly (2018-02-04).
1) Add the following search engine:

<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">
	<ShortName>Startpage</ShortName>
	<Description>Startpage Web Search</Description>
	<InputEncoding>UTF-8</InputEncoding>
	<Image width="16" height="16">
		data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAABHNCSVQICAgIfAhkiAAAAnZJREFUOE+lU01rE1EUPW/em3ynwdZWkpak1NIg2ioS3FjcVHAhuii4ELrRPyD0b7gruBIU7KoiFSsU6kehVDFSMEQIFTVUm6Ah0Zim+ZhJMvO8bxLRTRfSy8y8O+++e++Zc8+wdHZf4hAmGGOHSAcE1yhfFdHIkT0w6t22wSApRE/adxqpcK+frc7SJVQFaTYgU68hsxknCbEJsNPn0fYG8Tm/h8hRH0q/msgV69SHYTQcQHQoAI0zukGdVu4Dq4sQfj94MASWfAbt6T2YDROv3hfwYiuPD7t7DtxKzcTqmxzS2TI4FRO8VkYntQn98hzEpWsE3QKqFWDvB7ig8gR1eNCP6aljcAkNbUtiM13A1nYJY4REE4EAeDgKa/0x7LUlMPoMlajHxmnVIIib45EgfB7hwPe4NJydGHCIz5fqEHogCH5jHubaI7TXn6C98gCsfwjeq3PgE+d6/BJUSrYdAhmCPp0KcTRNi0gk8ll0FK6b85C1KqxCHsbzZRiLC2C3bjvTEUSWLhgsm15pGtW6BbNtoc+vE4mtBqzMO2hGDa7+I/BOTsI3cwWyZUBr7lOSxJdCd3XpauYSqU9lZ5rjw0FC1qyh/vCuM2v3VAKMCzTfbsBz8gw6IzHwTA5fC3UsvdzBQMiNYsXAzrcaLibCGBn0gRktW3aK39FIbqC1m3XIcY/F4Z+ewU/pw53lbcxeiJGubHzMVWl0Gk6MhhCP9nXHSNzAHY7APXu9q7QuT2CkJ63cdAp6iflTY/1IxBX7lKBESYiVGEkbPW0qmf4x8pVyOZGnomp8yv79bxxfbVOlA81odWQyU5KkvgPPMBX52/r/vd991RwkQ92omQAAAABJRU5ErkJggg==
	</Image>
	<Url type="text/html" method="POST" template="https://www.startpage.com/do/search">
		<Param name="query" value="{searchTerms}" />
	</Url>
	<SearchForm>https://www.startpage.com/</SearchForm>
</SearchPlugin>

2) Set "about:newtab" as Home Page in Preferences -> General
3) Restart
4) Start Network Monitor (Ctrl+Shift+E) to observe network requests.
5) Enter any search term in the awesome bar and search with the added "StartPage" search engine.
6) Do another search in the same way.


Actual results:

The first request is (incorrectly) sent as GET to https://www.startpage.com/do/search
This request gets redirected to https://www.startpage.com/ because it has no search parameters set.

The second request is correctly sent as POST with the "query" parameter set to the search terms. This results in a proper web search.



Expected results:

The first request should have been a POST request with the proper parameters set.

Comment 1

[Drew Willcoxon :adw]

Very strange, it does seem to happen only after setting about:newtab as your home page.

Comment 2

[Kidhanis]

The search fails when it's done in an unused and non-preloaded New Tab Page with e10s enabled.

If you set browser.newtab.preload to "false", every first search done in a new tab will fail.

The output of mozregression is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dd4bf41ca47d3e3036b194a61af4983c58dd14b2&tochange=1bc06a4d147320b0976467a9771ec4411dbe66d1, and it seems that https://hg.mozilla.org/mozilla-central/rev/1bc06a4d1473 is the only changeset relevant to this bug.

The condition in https://hg.mozilla.org/mozilla-central/rev/1bc06a4d1473#l4.12 gets satisfied when a search is done from an unused and non-preloaded New Tab Page, which means that E10SUtils.shouldLoadURI will return "false".  On the other hand, the condition is not satisfied with a preloaded New Tab Page because the introduction of the "isBrowserPreloaded" attribute in this changeset (later changed to "preloadedState") causes webNav.currentURI.spec to be "about:blank". Isn't the comment right under the condition suggesting that the preloaded New Tab Page should be the one that satisfies this condition?

E10SUtils.shouldLoadURI is called in https://hg.mozilla.org/mozilla-central/file/1bc06a4d1473/browser/base/content/tab-content.js#l717. Here, WebBrowserChrome.shouldLoadURI has aHasPostData, which indicates whether there is POST data or not.  Once E10SUtils.shouldLoadURI returns "false" for the non-preloaded New Tab Page, the POST data is essentially discarded since E10SUtils.redirectLoad does not send any POST data. This is what creates, in the case of the Startpage search engine, the GET request with no search query to https://www.startpage.com/do/search.

Comment 3

[Drew Willcoxon :adw]

Kidhanis, thanks for your debugging!

Comment 4

[Drew Willcoxon :adw]

This deserves to be higher than a P3 I think.

Comment 5

[Drew Willcoxon :adw]

Mike, can we just remove this part of bug 1376895's fix, at least for now?

https://hg.mozilla.org/mozilla-central/rev/1bc06a4d1473#l4.12

There are a couple of problems with it.  Comment 2 has details.  In summary:

(1) The "redirect this request" part of this doesn't pass along any POST data, which screws up POST searches when about:newtab is the current browser.  I tried to follow the code path that ultimately ends up loading the URI, and it looks like it happens in SessionStore and ContentRestore?  But there, too, it doesn't look like POST data is handled.

(2) As Kidhanis points out, for preloaded browsers, it appears that webNav.currentURI.spec is about:blank, not about:newtab.  That's fortuitous from my POV because it means that searches with POST data end up happening correctly when you do them in a preloaded browser.  But it doesn't seem consistent with what bug 1376895 was trying to do.

I'd be happy to try to fix this in a better way than removing this code chunk, but it seems like a bit of work, and I don't have much context for it.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Thanks for the needinfo - I've dug at this a bit.

(In reply to Drew Willcoxon :adw from comment #5)
> Mike, can we just remove this part of bug 1376895's fix, at least for now?
> 
> https://hg.mozilla.org/mozilla-central/rev/1bc06a4d1473#l4.12
> 

Unfortunately, no. Doing so would mean that when browsing away from about:newtab by clicking on a link within about:newtab, we'll continue to use the same process. Since the preloaded about:newtab always uses a pre-existing process, this would cause us to essentially sidestep e10s-multi in the common case. Most users would then be using a single content process.

The problem, generally speaking, is that POST requests and POST data don't seem to survive process switches - and this is something that appears to be known. Bug 1347983 bypassed one of these cases with the large allocation header, and we might be able to do something similar. Bug 1348018 and bug 1344465 are similar in nature, and bug 1344465 even has a few work-in-progress patches (though they're a bit old).

So given that Search folk think this is a P1, I can suggest a bandaid fix:

Adjust https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/toolkit/modules/E10SUtils.jsm#240 so that it checks aHasPostData and remoteType too. So the resulting conditional would be:

    if (!aHasPostData &&
        Services.appinfo.remoteType == WEB_REMOTE_TYPE &&
        sessionHistory.count == 1 &&
        webNav.currentURI.spec == "about:newtab") {
      return false;
    }

This means that in the case where the about:newtab is not preloaded, if we have POST data, we'll skip this conditional, and continue to load in the same process.

Does that seem acceptable, adw?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(Though, ultimately, I think fixing bug 1348018 and / or bug 1344465 is the better long-term fix)

Comment 8

[Drew Willcoxon :adw]

Sounds like that will work.  I'll get a patch together.  Thanks Mike!

Comment 9

[Drew Willcoxon :adw]

Attachment: Bug 1435615 - Reuse the same process when loading URIs on about:newtab when requests have POST data.

Review commit: https://reviewboard.mozilla.org/r/240186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240186/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8971443 [details]
Bug 1435615 - Reuse the same process when loading URIs on about:newtab when requests have POST data.

https://reviewboard.mozilla.org/r/240186/#review246128

Thanks!

::: commit-message-dfb15:1
(Diff revision 1)
> +Bug 1435615 - One-Click search engine uses GET instead of POST the first time after each start. r?mconley

We should probably update the commit message to talk about what's happening, and not the bug that we're fixing.

::: toolkit/modules/E10SUtils.jsm:248
(Diff revision 1)
> +        webNav.currentURI.spec == "about:newtab") {
>        // This is possibly a preloaded browser and we're about to navigate away for
>        // the first time. On the child side there is no way to tell for sure if that
>        // is the case, so let's redirect this request to the parent to decide if a new
> -      // process is needed.
> +      // process is needed. But we don't currently properly handle POST data in
> +      // redirects, so if there is POST data, don't return false here.

Can you please file a new bug to correct this, and mark is as depending on bug 1344465? And then perhaps reference that new bug here? We should probably get this all fixed up once bug 1344465 gets fixed.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8971443 [details]
Bug 1435615 - Reuse the same process when loading URIs on about:newtab when requests have POST data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240186/diff/1-2/

Comment 13

[Drew Willcoxon :adw]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #11)
> Can you please file a new bug to correct this

I filed bug 1457520.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/912879eb4c60
Reuse the same process when loading URIs on about:newtab when requests have POST data. r=mconley

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/912879eb4c60

Comment 18

[QwertyChouskie]

Yep, seems fixed.

Comment 19

[Drew Willcoxon :adw]

Thanks

Comment 20

[Julien Cristau [:jcristau]]

Do we want this change on esr60 or should we wontfix for that branch?

Comment 21

[QwertyChouskie]

This is a pretty bad bug for anyone using a search engine that uses POST.  Uplift would be appreciated if possible.

Comment 22

[Drew Willcoxon :adw]

Attachment: ESR 60 patch

Seems OK to uplift.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a small patch that fixes a bad bug for some users. Users can work around the bug but there's no reason they would know how.

User impact if declined:
Users on ESR 60 that have search engines that use the POST method will continue to see this bug -- they won't be able to properly search using those engines when about:newtab is the selected tab

Fix Landed on Version:
61

Risk to taking this patch (and alternatives if risky):
Low risk, changes only one line to bypass a certain case when the user searches with a POST search engine

String or UUID changes made by this patch:
None

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8982040 [details] [diff] [review]
ESR 60 patch

fix a bug affecting some search plugins, approved for 60.1esr

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a41872ae8adb