1347983 - Forms submitted from a large-allocation page discard post data

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Forms which are submitted from a large-allocation page currently discard their post data., This is because the function `nsIWebBrowserChrome3::ShouldLoadURI` is unaware of whether or not the requested load has any associated POST data. 

This adds that knowledge, and a test to ensure that these requests are handled correctly by large-allocation pages.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

MozReview-Commit-ID: 7SEdTJN9Xd2

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add a test to ensure that forms submitted from a large-allocation page behave correctly

MozReview-Commit-ID: Af44H11AFMf

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8848137 [details] [diff] [review]
Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

>+    if (!E10SUtils.shouldLoadURI(aDocShell, aURI, aReferrer, aHasPostData)) {
>+      // XXX: Do we want to complain if we have post data but are still
>+      // redirecting the load? Perhaps a telemetry probe? Theoretically we
>+      // shouldn't do this, as it throws out data.
ok, the comment isn't about this bug.
Please file another bug and add the bug number somewhere here.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8848137 [details] [diff] [review]
> Part 1: Make forms submitted from a large-allocation page not leave the
> large-allocation process
> 
> >+    if (!E10SUtils.shouldLoadURI(aDocShell, aURI, aReferrer, aHasPostData)) {
> >+      // XXX: Do we want to complain if we have post data but are still
> >+      // redirecting the load? Perhaps a telemetry probe? Theoretically we
> >+      // shouldn't do this, as it throws out data.
> ok, the comment isn't about this bug.
> Please file another bug and add the bug number somewhere here.

Filed bug 1348018.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

MozReview-Commit-ID: 7SEdTJN9Xd2

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2c7853ae71
Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/38ecd019eceb
Part 2: Add a test to ensure that forms submitted from a large-allocation page behave correctly, r=smaug

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8848179 [details] [diff] [review]
Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

Approval Request Comment
[Feature/Bug causing the regression]: bug 1331083
[User impact if declined]: POST forms submitted on pages with the Large-Allocation header will not work correctly on 32-bit windows builds of Firefox.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Just landed on inbound
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: Makes change to feature which will affect a very small number of sites (only those which opt into the large allocation header). Actual changes made by the patch are fairly limited in scope.
[String changes made/needed]: None

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: BETA Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

I rebased the patches onto beta (apply these patches to beta instead of the original ones - they will apply cleanly)

MozReview-Commit-ID: 7SEdTJN9Xd2

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: BETA Part 2: Add a test to ensure that forms submitted from a large-allocation page behave correctly

MozReview-Commit-ID: Af44H11AFMf

Comment 10

[Marcia Knous [:marcia]]

Tracking 53/54/55+ for this regression, for the reasons outlined in Comment 7.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b2c7853ae71
https://hg.mozilla.org/mozilla-central/rev/38ecd019eceb

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8848179 [details] [diff] [review]
Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process

Fix a regression that POST forms submitted on pages with the Large-Allocation header won't work correctly and include tests. Aurora54+ & Beta53+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93fee9a2f837
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac9d58dd3c7c

Comment 14

[Carsten Book [:Tomcat]]

has problems to apply to beta

grafting 406008:93fee9a2f837 "Bug 1347983 - Part 1: Make forms submitted from a large-allocation page not leave the large-allocation process, r=smaug a=gchang"
merging browser/base/content/browser.js
merging browser/modules/E10SUtils.jsm
merging docshell/base/nsDocShell.cpp
warning: conflicts while merging browser/modules/E10SUtils.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging docshell/base/nsDocShell.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

could you take a look, thanks!

Comment 15

[Carsten Book [:Tomcat]]

never mind, took the beta patches

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c20c847c5540
https://hg.mozilla.org/releases/mozilla-beta/rev/ab5725326239

Comment 17

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 7) and the fact that this fix has automated coverage.