Closed
Bug 1305162
Opened 8 years ago
Closed 8 years ago
Allow requestBody in onBeforeRequest on release
Categories
(WebExtensions :: Request Handling, defect, P2)
Tracking
(firefox53 fixed)
People
(Reporter: andy+bugzilla, Assigned: kmag)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [webRequest] triaged [necko-backlog])
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
dragana
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
dragana
:
review+
mixedpuppy
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
dragana
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
For Kris or Giorgio to fill out based on bug 1201979 comment 30
Comment 1•8 years ago
|
||
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).
Flags: needinfo?(amckay)
Comment 2•8 years ago
|
||
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 :)
Flags: needinfo?(amckay)
Comment 3•8 years ago
|
||
need info'ing me to request rights for giorgio in bugzilla
Flags: needinfo?(sescalante)
Whiteboard: [webRequest] triaged
Comment 4•8 years ago
|
||
(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 :)
Flags: needinfo?(sescalante)
Reporter | ||
Updated•8 years ago
|
Blocks: webext-port-noscript
Reporter | ||
Comment 5•8 years ago
|
||
Anyone you can recommend to help us on this bug?
Flags: needinfo?(jduell.mcbugs)
Comment 6•8 years ago
|
||
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.
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [webRequest] triaged → [webRequest] triaged [necko-backlog]
Reporter | ||
Comment 7•8 years ago
|
||
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.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Comment 8•8 years ago
|
||
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.
Flags: needinfo?(kmaglione+bmo)
Comment 9•8 years ago
|
||
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.
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Guys, could you tell in which release is it gonna be available? Will we have it in Firefox 51 or 52?
Assignee | ||
Comment 14•8 years ago
|
||
It should hopefully be in 53. It definitely won't be in 51.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
mozreview-review |
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.
Attachment #8817729 -
Flags: review?(mixedpuppy) → review+
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8817728 [details]
Bug 1305162: Part 2 - Remove support for upload streams with embedded headers.
https://reviewboard.mozilla.org/r/97940/#review98702
Attachment #8817728 -
Flags: review?(mixedpuppy) → review+
Updated•8 years ago
|
Attachment #8817859 -
Flags: review?(bgrinstead) → review?(odvarko)
Comment 22•8 years ago
|
||
Comment on attachment 8817859 [details]
Bug 1305162: Part 1c - Fix devtools tests that expect headers in upload stream.
Honza is on PTO.
Attachment #8817859 -
Flags: review?(odvarko) → review?(poirot.alex)
Comment 23•8 years ago
|
||
mozreview-review |
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.
Attachment #8817859 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
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 25•8 years ago
|
||
mozreview-review |
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");
Reporter | ||
Updated•8 years ago
|
webextensions: --- → +
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8817728 [details]
Bug 1305162: Part 2 - Remove support for upload streams with embedded headers.
https://reviewboard.mozilla.org/r/97940/#review101266
Attachment #8817728 -
Flags: review?(dd.mozilla) → review+
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8817858 [details]
Bug 1305162: Part 1b - Properly serialize nsIMIMEInputStream in RemoteWebNavigation.
https://reviewboard.mozilla.org/r/98078/#review101272
Attachment #8817858 -
Flags: review?(dd.mozilla) → review+
Comment 28•8 years ago
|
||
mozreview-review |
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."
Attachment #8817727 -
Flags: review?(dd.mozilla) → review+
Comment 29•8 years ago
|
||
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.
Assignee | ||
Comment 30•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•8 years ago
|
||
mozreview-review |
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.
Attachment #8817859 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 37•8 years ago
|
||
mozreview-review-reply |
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 38•8 years ago
|
||
mozreview-review |
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
Attachment #8817859 -
Flags: review+
Assignee | ||
Comment 39•8 years ago
|
||
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
Comment 40•8 years ago
|
||
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
Assignee | ||
Comment 41•8 years ago
|
||
Sorry, I mistook that for an intermittent when I saw it on try.
Assignee | ||
Comment 42•8 years ago
|
||
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
Assignee | ||
Comment 43•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a44074ad3dbbb9dcfa561940aeb2f83158de6f05
Bug 1305162: Follow-up: Fix bustage. r=me
Comment 44•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter | ||
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 45•8 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•