Allow requestBody in onBeforeRequest on release

RESOLVED FIXED in Firefox 53

Status

P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: andy+bugzilla, Assigned: kmag)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

48 Branch
mozilla53
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [webRequest] triaged [necko-backlog])

Attachments

(5 attachments)

(Reporter)

Description

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

Comment 1

2 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

2 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

2 years ago
need info'ing me to request rights for giorgio in bugzilla
Flags: needinfo?(sescalante)
Whiteboard: [webRequest] triaged

Comment 4

2 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

2 years ago
Blocks: 1214733
(Reporter)

Comment 5

2 years ago
Anyone you can recommend to help us on this bug?
Flags: needinfo?(jduell.mcbugs)

Comment 6

2 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

2 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

2 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

2 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

2 years ago
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 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

2 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

2 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

2 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+
Attachment #8817859 - Flags: review?(bgrinstead) → review?(odvarko)
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 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

2 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 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

2 years ago
webextensions: --- → +

Comment 26

2 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

2 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

2 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

2 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

2 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 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

2 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 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

2 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
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

2 years ago
Sorry, I mistook that for an intermittent when I saw it on try.
(Assignee)

Comment 42

2 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
(Reporter)

Updated

2 years ago
Keywords: dev-doc-needed
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

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.