Use channel.asyncOpen2 within dom/browser-element/BrowserElementParent.js

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8698795 [details] [diff] [review]
bug_1232901_asyncopen2_dom_browser_element.patch

Taking https://bugzilla.mozilla.org/show_bug.cgi?id=1232430#c4 and splitting the patch into several subproblems.

(In reply to Aus Lacroix [:aus] from comment #4)
> The goal of grabbing the loadingPrincipal is to have access to the correct
> cookie jar and other session related information so that you can download
> content properly because, often, sites require sign-in which is verified via
> session data.

Thanks Aus, but in what circumstances do we end up with a 'null' principal. Can that ever be the case? Probably we can simplify that code.

As the code is right now we never end up with a nullPrincipal (at least not in debug code) because we have assertions in place that don't allow creating of a channel without a loadingPrincipal.
Flags: needinfo?(aus)
(Assignee)

Updated

3 years ago
Blocks: 1232430
No longer blocks: 1182535

Comment 2

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8698795 [details] [diff] [review]
> bug_1232901_asyncopen2_dom_browser_element.patch
> 
> Taking https://bugzilla.mozilla.org/show_bug.cgi?id=1232430#c4 and splitting
> the patch into several subproblems.
> 
> (In reply to Aus Lacroix [:aus] from comment #4)
> > The goal of grabbing the loadingPrincipal is to have access to the correct
> > cookie jar and other session related information so that you can download
> > content properly because, often, sites require sign-in which is verified via
> > session data.
> 
> Thanks Aus, but in what circumstances do we end up with a 'null' principal.
> Can that ever be the case? Probably we can simplify that code.
> 
> As the code is right now we never end up with a nullPrincipal (at least not
> in debug code) because we have assertions in place that don't allow creating
> of a channel without a loadingPrincipal.

Sounds like based on the fact that we have this assertion we could completely remove the extra code and assume we'll have a loadingPrincipal. Awesome! :)
Flags: needinfo?(aus)
(Assignee)

Comment 3

3 years ago
Created attachment 8700204 [details] [diff] [review]
bug_1232901_asyncopen2_dom_browser_element.patch

Hey Aus, thanks for the update. One last question. Unfortunately, I don't fully understand what the code is doing but it seems we do not perform any kind of network security checks. Since we are going to change the callsite to use asyncOpen2(), do we still want no security checks to be performed? Or let me rephrase that, what kind of security checks would be appropriate for that channel?
Attachment #8698795 - Attachment is obsolete: true
Attachment #8700204 - Flags: review?(aus)

Comment 4

3 years ago
Comment on attachment 8700204 [details] [diff] [review]
bug_1232901_asyncopen2_dom_browser_element.patch

Looks fantastic! I love ditching old code. :)
Attachment #8700204 - Flags: review?(aus) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Created attachment 8700204 [details] [diff] [review]
> bug_1232901_asyncopen2_dom_browser_element.patch
> 
> Hey Aus, thanks for the update. One last question. Unfortunately, I don't
> fully understand what the code is doing but it seems we do not perform any
> kind of network security checks. Since we are going to change the callsite
> to use asyncOpen2(), do we still want no security checks to be performed? Or
> let me rephrase that, what kind of security checks would be appropriate for
> that channel?

Thanks for the r+ but I would really like you to weigh in what kind of security checks we need to perform before opening that channel. Maybe you could explain what this code is doing, then we can easily come up with the right policy. Thanks!
Flags: needinfo?(aus)

Comment 6

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> > Created attachment 8700204 [details] [diff] [review]
> > bug_1232901_asyncopen2_dom_browser_element.patch
> > 
> > Hey Aus, thanks for the update. One last question. Unfortunately, I don't
> > fully understand what the code is doing but it seems we do not perform any
> > kind of network security checks. Since we are going to change the callsite
> > to use asyncOpen2(), do we still want no security checks to be performed? Or
> > let me rephrase that, what kind of security checks would be appropriate for
> > that channel?
> 
> Thanks for the r+ but I would really like you to weigh in what kind of
> security checks we need to perform before opening that channel. Maybe you
> could explain what this code is doing, then we can easily come up with the
> right policy. Thanks!

Since this functionality in the browser element is used to support things such as 'save link as' and 'save image as' and the likes, I would imagine that SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS would be most correct while being the safest.
Flags: needinfo?(aus)

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0f2a2b6bbf62
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.