For Kris or Giorgio to fill out based on bug 1201979 comment 30

Basically this is about changing Necko's HTTP upload management code so that we can always reliably use channel.getRequestHeader() to retrieve the "Content-Type" header rather than having to parse it from the upload stream, like it's currently required in the most common case, i.e. HTML form submission. Side-note: where should I start to investigate why I cannot change anymore the status of any bug I did not report? For instance, I cannot assign myself this one (needinfo-ing Andy about this issue).

Regarding my editbugs privileges, I guess they've been unintentionally revoked, so I've asked them back through the canonical route, which I've just learned about from MDN (I don't remember there was such a thing 11 years ago, when I had them granted for the 1st time). We'll see :)

need info'ing me to request rights for giorgio in bugzilla

(In reply to :shell escalante from comment #3) > need info'ing me to request rights for giorgio in bugzilla I had them fixed during the week-end, everyhting is OK now. Thanks :)

Anyone you can recommend to help us on this bug?

It sounds like we've already got a code workaround for the admittedly unintuitive necko behavior in bug 1201979. If that's the case, I'm happy to put this on our necko backlog, but I'm not sure when we'll get to it.

The problem is that in order to get a sign off from a peer in bug 1201979, the code had a "if not release" flag. Landing the API in release for bug 1201979 is a priority for the Chrome parity work. But there's two ways to do this. One is if Jason can chat with Kris, maybe reviewing the work around and giving it a thumbs up or any suggested simple changes so it could go to release? Because we have the potential to break a few things here, we wanted to be cautious. Otherwise we'd need to land this change.

The main thing that's blocking it at this point is having a sane way to read headers from a nsIMIMEInputStream. The necko changes for that should be fairly simple, so if this is a high priority, I can write a patch for that.

I'm very sympathetic to the idea of having GetRequestHeader (or some alternative version of that function) return headers that are contained inside the upload body. Here are some issues in my mind about it: - We don't want to cause a significant performance hit for the common case. I.e. we don't want to slow down channels where no one ever asks for these headers, but we spend time parsing them anyway. OTOH, uploads are fairly infrequent, so maybe a little overhead isn't a big deal. - Are all upload streams rewindable? The nsIUploadChannel.idl comments seem to indicate that we can probably trust that that's the case. (If not I don't know how we're going to do this short of copying the entire stream as we parse it). - How to handle multipart: what do we return when the same header appears (but with different value: most obviously, Content-Length) in different parts of a multipart message? Maybe we should get together in Hawaii and talk over the API needs here and possible architectures for the fix--I'll organize something.

Guys, could you tell in which release is it gonna be available? Will we have it in Firefox 51 or 52?

It should hopefully be in 53. It definitely won't be in 51.

Comment on attachment 8817729 [details] Bug 1305162: Part 3 - Allow requestBody in release builds. https://reviewboard.mozilla.org/r/97942/#review98692 head_webrequest also needs the requestbody excpetion check removed.

Comment on attachment 8817728 [details] Bug 1305162: Part 2 - Remove support for upload streams with embedded headers. https://reviewboard.mozilla.org/r/97940/#review98702

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. Honza is on PTO.

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review99772 ::: devtools/client/netmonitor/test/browser_net_curl-utils.js:50 (Diff revision 1) > data = yield createCurlData(requests.multipartForm, gNetwork); > - testGetHeadersFromMultipartText(data); > > + testGetHeadersFromMultipartText({ > + postDataText: "Content-Type: text/plain\r\n\r\n", > + }); By hardcoding postDataText here you are no longer testing anything, you are asserting this hardcoded value instead of the result of createCurlData. I don't know exactly what changed, but you should change the assertion. Assert the headers if the information moved there instead of post-data.

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review99772 > By hardcoding postDataText here you are no longer testing anything, you are asserting this hardcoded value instead of the result of createCurlData. > > I don't know exactly what changed, but you should change the assertion. Assert the headers if the information moved there instead of post-data. As far as I can tell, the test was only testing the parsing method, not its interaction with the actual post data stream. The actual headers are tested for elsewhere, and since there are no longer any headers in the post data stream, the only alternative is to remove the tests for that function altogether.

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review100010 ::: devtools/client/netmonitor/test/browser_net_curl-utils.js:50 (Diff revision 1) > data = yield createCurlData(requests.multipartForm, gNetwork); > - testGetHeadersFromMultipartText(data); > > + testGetHeadersFromMultipartText({ > + postDataText: "Content-Type: text/plain\r\n\r\n", > + }); That's not my comprehesion of this test. Here, data.postDataText is: gNetwork.getString(requests.multipartForm.requestPostData.postData.text) (That is based on my comprehension of createCurlData) And requests.multipartForm seems to be refering to netmon UI. Now in the other test (browser_webconsole_netlogging.js) you now assert against the headers instead of postData. Can't you do the same here? If I understand both your patch and this test correctly, you should instead modify testGetHeadersFromMultipartText itself. And substitute: CurlUtils.getHeadersFromMultipartText(data.postDataText); With: CurlUtils.findHeader(headers, "Content-Type");

Comment on attachment 8817728 [details] Bug 1305162: Part 2 - Remove support for upload streams with embedded headers. https://reviewboard.mozilla.org/r/97940/#review101266

Comment on attachment 8817858 [details] Bug 1305162: Part 1b - Properly serialize nsIMIMEInputStream in RemoteWebNavigation. https://reviewboard.mozilla.org/r/98078/#review101272

Comment on attachment 8817727 [details] Bug 1305162: Part 1a - Separate nsIMIMEInputStream headers from stream data. https://reviewboard.mozilla.org/r/97938/#review101276 ::: netwerk/base/nsMIMEInputStream.cpp:100 (Diff revision 2) > { > NS_ENSURE_FALSE(mStartedReading, NS_ERROR_FAILURE); > - mAddContentLength = aAddContentLength; > + if (!aAddContentLength) { > + // Content-Length is automatically added by the channel when setting the > + // upload stream. > + return NS_ERROR_FAILURE; addContentLenght does not have any function any more? Please comment that. ::: netwerk/base/nsMIMEInputStream.cpp:114 (Diff revision 2) > > + HeaderEntry header; > + header.name().Append(aName); > + header.value().Append(aValue); > + > + if (!mHeaders.AppendElement(Move(header))) { you could create a empty HeaderEntry by calling: HeaderEntry *entry = mHeaders.Appendelement(); and fill the values instead using move. This is just a suggestion. ::: netwerk/protocol/http/HttpBaseChannel.cpp:121 (Diff revision 2) > +class CopyNonOriginHeaderVisitor final : public nsIHttpHeaderVisitor > +{ > +public: > + NS_DECL_ISUPPORTS > + > + explicit CopyNonOriginHeaderVisitor(nsIHttpChannel *aChannel) Can you give it a better name. This is adding headers, maybe call it AddHeadersToTheChannelHeaderVisitor , maybe you have a better idea. ::: netwerk/protocol/http/HttpBaseChannel.cpp:742 (Diff revision 2) > + mimeStream = do_QueryInterface(stream); > + if (mimeStream) { > + // Pass a void string for the content type so the Content-Type header > + // from the stream isn't overridden with an empty string. > + nsCString voidContentType; > + voidContentType.SetIsVoid(true); please move it just before use. The comment confused me a bit. The "an empty string" part is confusing. You actually just want to say "We need to pass a void string for the content type, because we do not want to change the Content-type header set by the stream."

Maybe you should updated page -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest Here is written: "'requestBody' is supported from version 50." But Kris Maglione [:kmag] 2016-12-10 14:11:38 PST It should hopefully be in 53. It definitely won't be in 51.

Comment on attachment 8817727 [details] Bug 1305162: Part 1a - Separate nsIMIMEInputStream headers from stream data. https://reviewboard.mozilla.org/r/97938/#review101276 > you could create a empty HeaderEntry by calling: > > HeaderEntry *entry = mHeaders.Appendelement(); > > and fill the values instead using move. > > This is just a suggestion. Done. Thanks. > Can you give it a better name. > This is adding headers, maybe call it AddHeadersToTheChannelHeaderVisitor , maybe you have a better idea. I changed it to `AddHeadersToChannelVisitor`

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review104610 ::: devtools/client/netmonitor/test/browser_net_curl-utils.js:52 (Diff revision 2) > data = yield createCurlData(requests.multipartForm, gNetwork); > - testGetHeadersFromMultipartText(data); > + testMultiPartHeaders(data); > + > + testGetHeadersFromMultipartText({ > + postDataText: "Content-Type: text/plain\r\n\r\n", > + }); You are still testing an hardcoded value here. Can you make it so that testGetHeadersFromMultipartText asserts against `data` instead of this { postDataText } hardcoded object? If there is no more headers in the multipart text, just remove this.

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review104610 > You are still testing an hardcoded value here. > Can you make it so that testGetHeadersFromMultipartText asserts against `data` instead of this { postDataText } hardcoded object? > If there is no more headers in the multipart text, just remove this. The only streams that contain embedded headers now come from deprecated NPAPI plugins, and possibly (but not likely) some legacy extensions. It's probably worth keeping `getHeadersFromMultipartText` for now, which means it still needs to be tested. But it no longer has any use for any requests generated by the browser, so the only way to test it is with mock stream data.

Comment on attachment 8817859 [details] Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. https://reviewboard.mozilla.org/r/98080/#review104630

https://hg.mozilla.org/integration/mozilla-inbound/rev/de131f7c1fc1f60db42509aea646ef4540e6c5fe Bug 1305162: Part 1a - Separate nsIMIMEInputStream headers from stream data. r=dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/1202edf6008d7c4da0976c78ed88cec7aa4f05d1 Bug 1305162: Part 1b - Properly serialize nsIMIMEInputStream in RemoteWebNavigation. r=dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/d1095e03dfc600771f54c7ff2dc5fd294ca1ab28 Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/1a23ce5a17c338a9ef902d4f30e586ad4444af36 Bug 1305162: Part 2 - Remove support for upload streams with embedded headers. r=mixedpuppy,dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/c633650caea682de11142d5cb6d28f3effcadf55 Bug 1305162: Part 3 - Allow requestBody in release builds. r=mixedpuppy

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/81824fdf77fb for permanent timeouts in e10s browser_action_keyword.js (Windows and Mac only since it's disabled on Linux), https://treeherder.mozilla.org/logviewer.html#?job_id=70898985&repo=mozilla-inbound

Sorry, I mistook that for an intermittent when I saw it on try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a28f51037094413286d828c1952af57d1a59322 Bug 1305162: Part 1a - Separate nsIMIMEInputStream headers from stream data. r=dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/63d6c884fe7b9c510eb2a90be390537c28f76fa3 Bug 1305162: Part 1b - Properly serialize nsIMIMEInputStream in RemoteWebNavigation. r=dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/d1afee5087d97c390e607fb973b614bd7e119f20 Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7a7af68b7522cc3886d369c40639098e73c4af Bug 1305162: Part 2 - Remove support for upload streams with embedded headers. r=mixedpuppy,dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/8812f239cc29e7c3460db4a62180bca0cf4bc921 Bug 1305162: Part 3 - Allow requestBody in release builds. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/a44074ad3dbbb9dcfa561940aeb2f83158de6f05 Bug 1305162: Follow-up: Fix bustage. r=me

https://hg.mozilla.org/mozilla-central/rev/7a28f5103709 https://hg.mozilla.org/mozilla-central/rev/63d6c884fe7b https://hg.mozilla.org/mozilla-central/rev/d1afee5087d9 https://hg.mozilla.org/mozilla-central/rev/2d7a7af68b75 https://hg.mozilla.org/mozilla-central/rev/8812f239cc29 https://hg.mozilla.org/mozilla-central/rev/a44074ad3dbb

Updated compat notes for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#Compatibility_notes I'm marking this dev-doc-complete, but please let me know if you need anything else.