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.

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

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.

Kidhanis, thanks for your debugging!

This deserves to be higher than a P3 I think.

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.

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?

(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!

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.

(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

Yep, seems fixed.

Thanks

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

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

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 on attachment 8982040 [details] [diff] [review] ESR 60 patch fix a bug affecting some search plugins, approved for 60.1esr