Closed Bug 1435615 Opened 2 years ago Closed Last year

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

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: PilzAdam, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(2 files)

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.
Component: Untriaged → Search
Very strange, it does seem to happen only after setting about:newtab as your home page.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [fxsearch]
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.
Blocks: 1376895
Keywords: regression
Kidhanis, thanks for your debugging!
See Also: → 1448517
This deserves to be higher than a P3 I think.
Priority: P3 → P1
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.
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(adw)
(Though, ultimately, I think fixing bug 1348018 and / or bug 1344465 is the better long-term fix)
Sounds like that will work.  I'll get a patch together.  Thanks Mike!
Flags: needinfo?(adw)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Duplicate of this bug: 1404653
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.
Attachment #8971443 - Flags: review?(mconley) → review+
See Also: → 1457520
(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.
Pushed by dwillcoxon@mozilla.com:
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
Blocks: 1441058
No longer blocks: 1441058
Duplicate of this bug: 1441058
Duplicate of this bug: 1448517
https://hg.mozilla.org/mozilla-central/rev/912879eb4c60
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Yep, seems fixed.
Thanks
Status: RESOLVED → VERIFIED
Do we want this change on esr60 or should we wontfix for that branch?
Flags: needinfo?(adw)
This is a pretty bad bug for anyone using a search engine that uses POST.  Uplift would be appreciated if possible.
Attached patch ESR 60 patchSplinter Review
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
Flags: needinfo?(adw)
Attachment #8982040 - Flags: approval-mozilla-esr60?
Comment on attachment 8982040 [details] [diff] [review]
ESR 60 patch

fix a bug affecting some search plugins, approved for 60.1esr
Attachment #8982040 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.