Closed Bug 1138454 Opened 9 years ago Closed 9 years ago

Green reftest R2 on Mulet

Categories

(Firefox OS Graveyard :: Runtime, defect)

x86_64
Linux
defect
Not set
normal

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)

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.
Or the real issue is that the data URL node gets identified as a certified app :)
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)
and just the removal to check for regressions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e388f18832c
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)
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)
(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)
Attachment #8571414 - Flags: review?(mozilla)
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+
Blocks: 1138895
Sorry, I fear you may need to backout this, as documented on bug 1138895 comment 1.
Flags: needinfo?(ryanvm)
Attachment #8573200 - Attachment is obsolete: true
No longer blocks: 1138895
Depends on: 1138895
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S8 (20mar)
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)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: