Closed
Bug 1138454
Opened 9 years ago
Closed 9 years ago
Green reftest R2 on Mulet
Categories
(Firefox OS Graveyard :: Runtime, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S8 (20mar)
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
(Whiteboard: [systemsfe])
Attachments
(1 file, 1 obsolete file)
2.99 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
Reftest R2 tests are failing because of the use of data URL for CSS, and those gets blocked. A hack shows green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57a83624dec&exclusion_profile=false Investigation shows that we do load the data URL with a NULL principal in |Loader::LoadInlineStyle|. That makes |CSPService::ShouldLoad| failing to find a CSP attached to the node and not granting permission to load.
Assignee | ||
Comment 1•9 years ago
|
||
Or the real issue is that the data URL node gets identified as a certified app :)
Assignee | ||
Comment 2•9 years ago
|
||
So the comment in [https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#146] states that the fast path should be removed once bug 925004 lands. Which happened a couple of months ago already. Can we actually remove it?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
on top of everything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a50fb992036d
Assignee | ||
Comment 5•9 years ago
|
||
and just the removal to check for regressions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e388f18832c
Comment 6•9 years ago
|
||
Alexandre, first thanks for picking up that work. I am really happy to see that happening. To answer your question up front: YES, we should definitely remove the fast path from CSP. Second, data URLs are not subject to CSP [1]. CSP would ACCEPT the load even before it gets down to the fast-path part, so that can't be the issue here. Third, there is an information overhead in this bug at the moment. I am very willing to help because I finally would love to see the 'fast-path' removed, but at the moment, I don't know what your actual question is. Please needinfo me again for further questions. [1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#132
Flags: needinfo?(mozilla)
Assignee | ||
Comment 7•9 years ago
|
||
Well, according to http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#53, it is explicitely stated that data URL are subject to CSP. Who is right there ? Removing the fast path does help and try seems to be happy, so it looks like we can safely drop this. If there are further corrections needed just tell me :)
Flags: needinfo?(mozilla)
Comment 8•9 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #7) > Well, according to > http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService. > cpp#53, it is explicitely stated that data URL are subject to CSP. Who is > right there ? Facepalm, you are absolutely right, shouldn't answer bugmail before noon :-) > Removing the fast path does help and try seems to be happy, so it looks like > we can safely drop this. If there are further corrections needed just tell > me :) Yeah, please flag me for review, happy to have the fastpath removed.
Flags: needinfo?(mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8571414 -
Flags: review?(mozilla)
Comment 9•9 years ago
|
||
Comment on attachment 8571414 [details] [diff] [review] Remove CSP fast path hack for certified apps r=ckerschb Review of attachment 8571414 [details] [diff] [review]: ----------------------------------------------------------------- If TRY passes, then all good! So happy to see the fast-path finally being removed!
Attachment #8571414 -
Flags: review?(mozilla) → review+
Assignee | ||
Comment 10•9 years ago
|
||
That looks green enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e388f18832c
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/deeb2d276a85
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
Sorry, I fear you may need to backout this, as documented on bug 1138895 comment 1.
Flags: needinfo?(ryanvm)
Comment 13•9 years ago
|
||
Indeed. https://hg.mozilla.org/integration/mozilla-inbound/rev/f1df4a83c72b
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8573200 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 15•9 years ago
|
||
Green and stable: https://treeherder.allizom.org/#/jobs?repo=try&revision=4b11f6df3a24&exclusion_profile=false https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b11f6df3a24&exclusion_profile=false
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S8 (20mar)
Comment 16•9 years ago
|
||
I am slightly confused what happened here actually, didn't we agree on removing the CSP fast-path? That's also what I r+ed, no?
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16) > I am slightly confused what happened here actually, didn't we agree on > removing the CSP fast-path? That's also what I r+ed, no? But this proved to be breaking some other B2G tests as documented in comment 12. And bug 1138895 was the real proper fix for this, that do not breaks anything.
Flags: needinfo?(lissyx+mozillians)
You need to log in
before you can comment on or make changes to this bug.
Description
•