Closed
Bug 1274509
Opened 8 years ago
Closed 8 years ago
Do nsHttpRequestHead::VisitHeaders outside lock
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: dragana, Assigned: dragana)
Details
(Keywords: dev-doc-complete, Whiteboard: [necko-active])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [necko-active]
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8754753 -
Flags: review?(mcmanus)
Comment 4•8 years ago
|
||
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
Attachment #8754753 -
Flags: review?(mcmanus)
Assignee | ||
Comment 5•8 years ago
|
||
(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
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
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.
Attachment #8754753 -
Attachment is obsolete: true
Attachment #8755298 -
Flags: review?(mcmanus)
Updated•8 years ago
|
Attachment #8755298 -
Flags: review?(mcmanus) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 9•8 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=28532193&repo=mozilla-inbound
Flags: needinfo?(dd.mozilla)
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8755298 -
Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8755821 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 14•8 years ago
|
||
For dev-doc-needed: I have only change that setHeaders, referrer(just setter) and setReferrerWithPolicy will return an error if called during VisitHeaders.
Keywords: dev-doc-needed
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 16•8 years ago
|
||
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
Flags: needinfo?(dd.mozilla)
Assignee | ||
Comment 17•8 years ago
|
||
(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.
Flags: needinfo?(dd.mozilla)
Comment 18•8 years ago
|
||
(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
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 19•8 years ago
|
||
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
Attachment #8755821 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8755821 [details] [diff] [review]
bug_1274509_v2.patch
needs a rebase :)
Attachment #8755821 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•8 years ago
|
||
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
Attachment #8759934 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch
Switching the flag so it's for 48
Attachment #8759934 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment 23•8 years ago
|
||
Comment on attachment 8759934 [details] [diff] [review]
bug_1274509_v2_aurora.patch
Take it for the bug 1247982
Should be in 48 beta 3
Attachment #8759934 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 24•8 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 25•8 years ago
|
||
: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.
Flags: needinfo?(sebastianzartner)
Comment 26•8 years ago
|
||
I've updated the doc. Please verify that it's correct.
Sebastian
Flags: needinfo?(sebastianzartner) → needinfo?(dd.mozilla)
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #26)
> I've updated the doc. Please verify that it's correct.
>
> Sebastian
Looks good, thanks!!!
Flags: needinfo?(dd.mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•