Use channel->ascynOpen2 in dom/base/nsObjectLoadingContent.cpp

RESOLVED FIXED in Firefox 50



4 years ago
2 years ago


(Reporter: ckerschb, Assigned: ckerschb)


Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)


(Whiteboard: [domsecurity-active])


(1 attachment, 1 obsolete attachment)

Comment hidden (empty)


4 years ago
Assignee: nobody → mozilla
Blocks: 1182535


3 years ago
Depends on: 1206961, 1182569
Created attachment 8664046 [details] [diff] [review]

Putting up a WIP patch since this bug is still blocked by others, but the question is: What do we do about NS_CheckContentProcessPolicy in general?
Flags: needinfo?(jonas)
Regarding NS_CheckContentProcessPolicy, I would say that we can either leave the checks as-is, or try to move them into the channel. I don't know if the checks are called at a consistent enough place that it makes sense to move into the channel. I.e. if the checks are always happening in OnStartRequest or if they always happen before the channel is opened.

If they are too inconsistent I would say leave them as-is.
Flags: needinfo?(jonas)
Some old notes I took on ShouldProcess() can be found on line 59 onwards here:

I don't fully understand the use case for it and am not convinced that our implementation is sound.
I share Tanvi's concern that our ShouldProcess implementation is probably not sound. All the more reason to try to fix/remove it as a separate effort in separate bugs.
(In reply to Jonas Sicking (:sicking) from comment #4)
> I share Tanvi's concern that our ShouldProcess implementation is probably
> not sound. All the more reason to try to fix/remove it as a separate effort
> in separate bugs.

I agree with both of you. What I remember though, addons are still using them, so I think it's just safest if we leave them as-is for now. Once we are done with moving security checks into AsyncOpen2, we can probably start to deprecate them in a separate effort.


3 years ago
Whiteboard: [domsecurity-active]
Created attachment 8765571 [details] [diff] [review]

Hey Olli, since :bz is on PTO I was wondering if you feel comfortable reviewing a change within nsObjectLoadingContent or if you could suggest someone else who might be willing to review that change.

We have *not* converted nsURILoader to use asyncOpen2() yet (see Bug 1182569), so we can't remove all relevant security checks within nsObjectLoadingContent, because as you can see there is a call to uriloader->OpenChannel() within nsObjectLoadingContent(). Hence I would like to keep this changeset as self contained as possible so that security checks are only skipped at the callsite and enforced within .asyncOpen2() if we are in fact hitting that branch where we create a new channel and call asyncOpen2() on it. Happy to answer more questions if that description does not provide sufficient information.
Attachment #8664046 - Attachment is obsolete: true
Attachment #8765571 - Flags: review?(bugs)
Attachment #8765571 - Flags: review?(bugs) → review+

Comment 8

3 years ago
Pushed by
Use channel->ascynOpen2 in dom/base/nsObjectLoadingContent.cpp r=smaug

Comment 9

3 years ago
Last Resolved: 3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1317641
You need to log in before you can comment on or make changes to this bug.