Closed
Bug 1384493
Opened 7 years ago
Closed 7 years ago
dom/base/test/test_link_stylesheet.html fails when we enable rcwn, due to speculative loading style
Categories
(Core :: Security, enhancement)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: CuveeHsu, Assigned: tnguyen)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
19.26 KB,
patch
|
tnguyen
:
review+
|
Details | Diff | Splinter Review |
8.15 KB,
patch
|
tnguyen
:
review+
|
Details | Diff | Splinter Review |
RCWN would break the test by the following command mach mochitest --setpref network.http.rcwn.enabled=true dom/base/test/test_link_stylesheet.html bug 1358038 comment 0 has a brief of RCWN. RCWN might: a) get some resource from net instead of cache b) remove If-Modified-Since/If-Match header Hello Thomas, Do you think the above behaviors would change the test? If yes, we might disable RCWN in this test. Otherwise, we should find the bug.
Flags: needinfo?(tnguyen)
Assignee | ||
Comment 1•7 years ago
|
||
I think the test should be passed as expected. The loading resource request should use the correct referrer/referrer policy. I tried to run the test locally and it failed randomly (intermittent). Looks like the request use document's referrer policy instead of referrerPolicy attribute in the link element. Plz see http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/dom/base/nsStyleLinkElement.cpp#557 We got the correct policy here, but somewhere, we probably sent request with another policy. We should find that place and correct it by using GetLinkReferrerPolicy(). If fixing that is going to delay your work, please disable the test temporarily. We should use this bug to keep track of that issue.
Flags: needinfo?(tnguyen)
Reporter | ||
Comment 2•7 years ago
|
||
Thanks for your prompt response! The following command might make failure more deterministic. mach mochitest --disable-e10s --setpref network.http.rcwn.enabled=true --setpref network.http.rcwn.max_wait_before_racing_ms=0 dom/base/test/test_link_stylesheet.html assign myself to investigate
Assignee: nobody → juhsu
Whiteboard: [necko-active]
Assignee | ||
Comment 3•7 years ago
|
||
Well, we did a speculative loading that does not follow the referrerpolicy in attribute The race condition in network.http.rcwn.enabled will decide if we need to use the second request network, then we failed randomly. We have to fix that in speculative loading style.
Summary: dom/base/test/test_link_stylesheet.html fails when we enable rcwn → Speculative loading style should use correct referrer policy
Assignee | ||
Updated•7 years ago
|
Summary: Speculative loading style should use correct referrer policy → dom/base/test/test_link_stylesheet.html fails when we enable rcwn, due to speculative loading style
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Component: Networking: Cache → Security
Assignee | ||
Updated•7 years ago
|
Assignee: juhsu → tnguyen
Assignee | ||
Updated•7 years ago
|
Attachment #8891208 -
Flags: review?(wchen)
Assignee | ||
Comment 8•7 years ago
|
||
I uploaded a patch to correct referrer policy when loading style (preloading and some places which may parse referrerpolicy attribute) Hi, wchen, heycam. Could you please take a look at the patch? Thanks
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8891209 [details] Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy https://reviewboard.mozilla.org/r/162416/#review167684 Hi Thomas, some questions. ::: dom/base/nsContentSink.cpp:656 (Diff revision 1) > + // https://html.spec.whatwg.org/multipage/urls-and-fetching.html#referrer-policy-attribute > + // Specs says referrer policy attribute is an enumerated attribute, > + // case insensitive and includes the empty string > + if (referrerPolicy.IsEmpty()) { > + referrerPolicy = value; > + referrerPolicy.StripWhitespace(); Is StripWhitespace() really the right thing to call? That will turn e.g. " or i gin " into "origin", not just strip it from the start/end. But in any case, HTML enumerated attributes don't allow white space in their values at all. So I think we should just avoid stripping and allow AttributeReferrerPolicyFromString to reject strings that have white space in them. The other uses of StripWhitespace() for the other Link header attributes above look suspicious, too! Can you please file a bug for that? (Perhaps there is some legacy need to allow random white space in these attributes but I don't know.) ::: dom/base/nsStyleLinkElement.cpp:526 (Diff revision 1) > + // if referrer attributes are enabled in preferences, load the link's referrer > + // attribute. If the link does not provide a referrer attribute, ignore this > + // and use the document's referrer policy This comment seems wrong / out of date. Can you update it to talk about referrerpolicy="" instead? ::: dom/base/nsStyleLinkElement.cpp:530 (Diff revision 1) > + net::ReferrerPolicy referrerPolicy = GetLinkReferrerPolicy(); > + if (referrerPolicy == net::RP_Unset) { > + referrerPolicy = doc->GetReferrerPolicy(); > + } It seems like we do this "fall back to the document's referrer policy if not specified on the element" thing twice: once here, and then inside Loader::LoadInlineStyle. Is that really needed? ::: dom/xml/nsXMLContentSink.cpp:1283 (Diff revision 1) > > nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::title, aTitle); > > nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::media, aMedia); > > + nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::referrer, aMedia); Is there a spec that says <?xml-stylesheet?> processing instructions should support a referrer="" attribute? And if so, why is it referrer="" and not referrerpolicy="" like on <link> etc.?
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891209 [details] Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy https://reviewboard.mozilla.org/r/162416/#review167684 > Is StripWhitespace() really the right thing to call? That will turn e.g. " or i gin " into "origin", not just strip it from the start/end. But in any case, HTML enumerated attributes don't allow white space in their values at all. So I think we should just avoid stripping and allow AttributeReferrerPolicyFromString to reject strings that have white space in them. > > The other uses of StripWhitespace() for the other Link header attributes above look suspicious, too! Can you please file a bug for that? (Perhaps there is some legacy need to allow random white space in these attributes but I don't know.) You are right, https://html.spec.whatwg.org/multipage/semantics.html#the-link-element crossorigin should not be use StripWhitespace too. > It seems like we do this "fall back to the document's referrer policy if not specified on the element" thing twice: once here, and then inside Loader::LoadInlineStyle. Is that really needed? You are right. Because we added aReferrerPolicy parameter, will remove the check condition in LoadInlineStyle > Is there a spec that says <?xml-stylesheet?> processing instructions should support a referrer="" attribute? And if so, why is it referrer="" and not referrerpolicy="" like on <link> etc.? Thank you for pointing out that , no explicit specs talks about that. I think should use EmptyString() and use document'referrer policy instead. Thanks
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8891209 [details] Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy https://reviewboard.mozilla.org/r/162416/#review167684 > You are right, https://html.spec.whatwg.org/multipage/semantics.html#the-link-element > crossorigin should not be use StripWhitespace too. the spec http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading-and-trailing-whitespace seems out of date. I will not use CompressWhitespace and keep all white spaces
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8891209 [details] Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy https://reviewboard.mozilla.org/r/162416/#review167710 r=me with these comments addressed. ::: dom/base/nsContentSink.cpp:651 (Diff revision 2) > + // https://html.spec.whatwg.org/multipage/urls-and-fetching.html#referrer-policy-attribute > + // Specs says referrer policy attribute is an enumerated attribute, > + // case insensitive and includes the empty string Maybe note in the comment here that we will parse this value with AttributeReferrerPolicyFromString later, which will handle parsing it as an enumerated attribute. ::: dom/base/nsStyleLinkElement.cpp:526 (Diff revision 2) > + // if referrer attributes are enabled in preferences, load the link's > + // referrerpolicy attribute. If the link does not provide a referrerpolicy > + // attribute, ignore this and use the document's referrer policy "if referrer attributes are enabled in preferences" -- are there still actually such a preference? Looking through GetLinkReferrerPolicy, I don't see it doing any pref checks. If that part of the sentence is out of date too, please remove it. Also, consider duplicating the comment in nsContentSink::ProcessLink. ::: dom/xml/nsXMLContentSink.cpp:1256 (Diff revision 2) > type.IsEmpty() || > type.LowerCaseEqualsLiteral("text/css")) { > return DidProcessATokenImpl(); > } > > - nsAutoString href, title, media; > + nsAutoString href, title, media, referrerPolicy; Nit: remove referrerPolicy. ::: dom/xml/nsXMLContentSink.cpp:1264 (Diff revision 2) > // If there was no href, we can't do anything with this PI > - if (!ParsePIData(data, href, title, media, isAlternate)) { > + if (!ParsePIData(data, href, title, media,isAlternate)) { > return DidProcessATokenImpl(); > } > > - rv = ProcessStyleLink(node, href, isAlternate, title, type, media); > + rv = ProcessStyleLink(node, href, isAlternate, title, type, media, EmptyString()); Maybe comment here that <?xml-stylesheet?> processing instructions don't have a referrerpolicy pseudo-attribute, so we pass in an empty string for it. ::: layout/style/Loader.h:231 (Diff revision 2) > * @param aMedia the media string for the sheet. > * @param aObserver the observer to notify when the load completes. > * May be null. Can you add a comment in here documenting the argument?
Attachment #8891209 -
Flags: review?(cam) → review+
Comment 14•7 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12) > the spec > http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading- > and-trailing-whitespace seems out of date. > I will not use CompressWhitespace and keep all white spaces Please use the WHATWG HTML spec as a reference instead. :-)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8891209 [details] Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy https://reviewboard.mozilla.org/r/162416/#review167714 Updated the patch
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8891208 [details] Bug 1384493 - Speculative loading style should use correct referrer policy https://reviewboard.mozilla.org/r/162414/#review168452
Attachment #8891208 -
Flags: review?(william) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d74909e36c13e040586054a4552af1bda8be7a1
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 22•7 years ago
|
||
The first patch doesn't meet the review requirements for Autoland to push it. http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
Assignee | ||
Comment 23•7 years ago
|
||
I see "A suitable reviewer has not given a "Ship It!" in the patch even html/parser peer gave a r+ Hi William, could you please take a look again to see if you could change the flag? Or there is a potential bug in review board system. Thanks for your review.
Flags: needinfo?(william)
Assignee | ||
Comment 24•7 years ago
|
||
When preloading style link in <head>, we used speculative referrer policy which is delievered from meta tag and ignored the referrerpolicy attribute. We should use referrerpolicy attribute with the higher priority MozReview-Commit-ID: 1rQmBV01jvV
Assignee | ||
Comment 25•7 years ago
|
||
If the link element has referrerpolicy attribute, we should use policy in the attribute with higher priority MozReview-Commit-ID: GZZeRaoxPUw
Assignee | ||
Updated•7 years ago
|
Attachment #8891208 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8891209 -
Attachment is obsolete: true
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8897314 [details] [diff] [review] Speculative loading style should use correct referrer policy Change to use raw patch due to potential bug in reviewboard
Attachment #8897314 -
Flags: review+
Assignee | ||
Comment 27•7 years ago
|
||
If the link element has referrerpolicy attribute, we should use policy in the attribute with higher priority MozReview-Commit-ID: GZZeRaoxPUw
Assignee | ||
Comment 28•7 years ago
|
||
When preloading style link in <head>, we used speculative referrer policy which is delievered from meta tag and ignored the referrerpolicy attribute. We should use referrerpolicy attribute with the higher priority MozReview-Commit-ID: 1rQmBV01jvV
Assignee | ||
Updated•7 years ago
|
Attachment #8897314 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897315 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897318 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(william)
Attachment #8897319 -
Flags: review+
Assignee | ||
Comment 29•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e8f1d8d4d249a9eab8b10114c45180cbf75545
Keywords: checkin-needed
Comment 30•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/19a77f844150 Speculative loading style should use correct referrer policy. r=wchen https://hg.mozilla.org/integration/mozilla-inbound/rev/53e3c926ff03 LoadStyleLink and LoadInlineStyle should use correct referrer policy. r=heycam
Keywords: checkin-needed
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19a77f844150 https://hg.mozilla.org/mozilla-central/rev/53e3c926ff03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Updated•7 years ago
|
See Also: → CVE-2017-7842
You need to log in
before you can comment on or make changes to this bug.
Description
•