Closed Bug 1254320 Opened 6 years ago Closed 6 years ago

Remove SEC_NORMAL from dom/json/

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
Boris, sooner or later we should not use SEC_NORMAL as a securityFlag anymore, but rather only one of the 5 flags defined here [1]. I honestly can't recall why we decided to use a nullPrincipal as the loadingPrincipal in the first place [2]. Should we rather use the systemPrincipal as the loadingPrincipal?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#41
[2] http://hg.mozilla.org/mozilla-central/rev/7921e44f3544#l1.3
Attachment #8727628 - Flags: review?(bzbarsky)
Comment on attachment 8727628 [details] [diff] [review]
bug_1254320_remove_sec_normal_from_dom_json.patch

Let's back up a sec... why does this this code even use a channel?  I see absolutely no reason for it to do so.  Maybe it meant to work with other sorts of channels sometimes, but it doesn't right now.

So really, it totally doesn't matter what we pass here (e.g. we never open this channel!).  But it sure would be nice to just stop doing this insanity in general instead of continuing to wallpaper over it.

I'm not sure whether to mark this "r+, it just doesn't matter" or "r-, please make this code sane"...  I guess I'll go with the former, but please do consider the latter!  If not, at least make sure a followup is filed.
Attachment #8727628 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #2)
> So really, it totally doesn't matter what we pass here (e.g. we never open
> this channel!).

Indeed, I forgot to mention that in my initial review request - we never open that channel, hence using the most restrictive securityflag is what we should use in case we keep that NewChannel code there.

It would definitely be nice to remove that code insanity and replace it with something nicer. If I don't have the time I'll definitely leave a comment in the code and file a follow up.
Added a comment; carrying over r+ from bz.
Attachment #8727628 - Attachment is obsolete: true
Attachment #8728010 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/241d48f051ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.