1274509 - Do nsHttpRequestHead::VisitHeaders outside lock

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

During nsHttpRequestHead::VisitHeaders nsHttpRequestHead is lock. This function is expose to outside of the internal gecko code. If any element of nsHttpRequestHead is access during VisitHeaders() call, it will deadlock. 

Maybe I am to cautious, but I was thinking that we should change this.

Comment 1

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd4a6e2207a

Comment 2

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6cdf53f3665

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1274509_v1.patch

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8754753 [details] [diff] [review]
bug_1274509_v1.patch

Review of attachment 8754753 [details] [diff] [review]:
-----------------------------------------------------------------

if the headers are copied then the visitor can't modify what the channel is using.. that's a major change, right?

maybe using PRMonitor (which you would probably need write a small autoclass for) instead of mutex would do the trick. They are re-entrant on the same thread.. that might be a little iffy, but it seems to me the problem we are chasing is due to cross thread interaction.

::: netwerk/protocol/http/nsHttpRequestHead.h
@@ +40,5 @@
>      void SetRequestURI(const nsCSubstring &s);
>      void SetPath(const nsCSubstring &s);
>      uint32_t HeaderCount();
>  
> +    // This functions is going to copy headers under a lock and intereate

s/functions/function

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> Comment on attachment 8754753 [details] [diff] [review]
> bug_1274509_v1.patch
> 
> Review of attachment 8754753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> if the headers are copied then the visitor can't modify what the channel is
> using.. that's a major change, right?
> 

The visitor was getting only a copy of two strings: header and value, so changing them have not had and will not have(with this patch) any effect. In docs there is a warning that SetHeader should not be used and hopefully nobody uses because VisitHeader is an for loop through header array and deleting a header would cause trouble. With this patch calling SetHeader will be fine. 

> maybe using PRMonitor (which you would probably need write a small autoclass
> for) instead of mutex would do the trick. They are re-entrant on the same
> thread.. that might be a little iffy, but it seems to me the problem we are
> chasing is due to cross thread interaction.
> 

I knew there is something - PRMonitor, but I could not find it today :)
I will change it to PRMonitor.


> ::: netwerk/protocol/http/nsHttpRequestHead.h
> @@ +40,5 @@
> >      void SetRequestURI(const nsCSubstring &s);
> >      void SetPath(const nsCSubstring &s);
> >      uint32_t HeaderCount();
> >  
> > +    // This functions is going to copy headers under a lock and intereate
> 
> s/functions/function

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3698cbeb5d07

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1274509_v2.patch

I have changed our behavior now to throw an error if someone tries to SetHeader while in VisitHeaders.

We will need to update docs as well.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d34a99623615

Comment 9

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=28532193&repo=mozilla-inbound

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c75ad684ce2

Comment 11

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eb8b61c9bee

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: bug_1274509_v2.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/697e260a0fd8

Comment 14

[Dragana Damjanovic [:dragana]]

For dev-doc-needed: I have only change that setHeaders, referrer(just setter) and setReferrerWithPolicy will return an error if called during VisitHeaders.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/697e260a0fd8

Comment 16

[Sebastian Zartner [:sebo]]

Documented this here:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel

Dragana, can you please have a look again whether the descriptions are correct?

Sebastian

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Sebastian Zartner [:sebo] from comment #16)
> Documented this here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel
> 
> Dragana, can you please have a look again whether the descriptions are
> correct?
> 
> Sebastian


Thanks.

As part of visitRequestHeaders() description there is a warning:
"Warning: Calling setRequestHeader() while visiting request headers has undefined behavior. Don't do it!"

can you please extend it that setRequestHeader(), setReferrerWithPolicy(), setEmptyRequestHeader() and setting referrer attribute will return error starting with gecko49.

Comment 18

[Sebastian Zartner [:sebo]]

(In reply to Dragana Damjanovic [:dragana] from comment #17)
> (In reply to Sebastian Zartner [:sebo] from comment #16)
> > Documented this here:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIHttpChannel
> > 
> > Dragana, can you please have a look again whether the descriptions are
> > correct?
> > 
> > Sebastian
> 
> 
> Thanks.
> 
> As part of visitRequestHeaders() description there is a warning:
> "Warning: Calling setRequestHeader() while visiting request headers has
> undefined behavior. Don't do it!"
> 
> can you please extend it that setRequestHeader(), setReferrerWithPolicy(),
> setEmptyRequestHeader() and setting referrer attribute will return error
> starting with gecko49.

Done. Thank you!

Sebastian

Comment 19

[Dragana Damjanovic [:dragana]]

Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch

Approval Request Comment
[Feature/regressing bug #]: neede for uplift of bug 1247982
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low risk this patch only changes Mutex to ReentrantMonitor
[String/UUID change made/needed]:

the bug changes an idl - setting header and setting referrer during VisitRequestHeader was an unsafe operation, really hope no one did it, now it is going to return an error

Order of uplift:
bug 1247982,
bug 1269738,
this patch

Comment 20

[Dragana Damjanovic [:dragana]]

Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch

needs a rebase :)

Comment 21

[Dragana Damjanovic [:dragana]]

Attachment: bug_1274509_v2_aurora.patch

Approval Request Comment
[Feature/regressing bug #]: needed for uplift of bug 1247982
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low risk this patch only changes Mutex to ReentrantMonitor
[String/UUID change made/needed]:

the bug changes an idl - setting header and setting referrer during VisitRequestHeader was an unsafe operation, really hope no one did it, now it is going to return an error

Order of uplift:
bug 1247982,
bug 1269738,
this patch

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch

Switching the flag so it's for 48

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch


Take it for the bug 1247982
Should be in 48 beta 3

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52db915c585a

Comment 25

[Dragana Damjanovic [:dragana]]

:sebo, may I ask you to update doc for this bug. It was uplifted to 48. Only requestHeader part was uplifted, the response header change has not been uplifted.

Thank you.

Comment 26

[Sebastian Zartner [:sebo]]

I've updated the doc. Please verify that it's correct.

Sebastian

Comment 27

[Dragana Damjanovic [:dragana]]

(In reply to Sebastian Zartner [:sebo] from comment #26)
> I've updated the doc. Please verify that it's correct.
> 
> Sebastian

Looks good, thanks!!!