Split GetChannelPrincipal into GetChannelResultPrincipal and GetChannelURIPrincipal

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Initially, the patch for this bug was part of Bug 1038756 - Extending LoadInfo and storing LoadInfo on all channels created through NS_NewChannel.

Since it might take a little longer till Bug 1038756 is ready to land, we can take that independent (and already reviewed) part and land it ahead of time.
(Assignee)

Updated

4 years ago
Blocks: 1038756
Pushing that part to try to make sure everything is fine:

> You can view the progress of your build at the following URL:
> https://tbpl.mozilla.org/?tree=Try&rev=054997d3e427
> Alternatively, view them on Treeherder (experimental):
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=054997d3e427
Created attachment 8483745 [details] [diff] [review]
bug_1062529_split_getchannelprincipal.patch

Carrying over r+ from bz, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c178
Attachment #8483745 - Flags: review+
(Assignee)

Updated

4 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Created attachment 8483745 [details] [diff] [review]
> bug_1062529_split_getchannelprincipal.patch
> 
> Carrying over r+ from bz, see:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c178

This looks good to me; can someone check that in for me please?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c00ae6dd85ec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
The patch here does more than just rename things, but there's no information about *why* this change was made. I think it was a good change, but it would be good to have documented why we did it.

Specifically, the behavioral change that was made was that the CORS code now calls GetChannelURIPrincipal, whereas it used to call GetChannelPrincipal (i.e. the same thing as the new GetChannelResultPrincipal)

I.e. why is it important that the CORS code uses GetChannelURIPrincipal? What problem was that fixing?
(In reply to Jonas Sicking (:sicking) from comment #6)
> The patch here does more than just rename things, but there's no information
> about *why* this change was made. I think it was a good change, but it would
> be good to have documented why we did it.
> 
> Specifically, the behavioral change that was made was that the CORS code now
> calls GetChannelURIPrincipal, whereas it used to call GetChannelPrincipal
> (i.e. the same thing as the new GetChannelResultPrincipal)
> 
> I.e. why is it important that the CORS code uses GetChannelURIPrincipal?
> What problem was that fixing?

We should have created that bug earlier, now all the conversation, discussion and documentation still lives in Bug 1038756 which turned out to have 250+ comments. Anyway, the interesting part about CORS starts here:
  https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c24
Chris, can you add comments to nsScriptSecurityManager with a summary?
(In reply to Tanvi Vyas [:tanvi] from comment #8)
> Chris, can you add comments to nsScriptSecurityManager with a summary?

Sure, doing that over in bug 1134096.
You need to log in before you can comment on or make changes to this bug.