If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

PrivateBrowsingChannel::SetPrivate should not die if channel has a load group with no load context

RESOLVED FIXED in mozilla24

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Mook, Assigned: Mook)

Tracking

Trunk
mozilla24
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIPrivateBrowsingChannel.idl?rev=cc6f5b3ca9de&mark=26-29#17 is kind of ambiguous; it says that nsIPrivateBrowsingChannel::setPrivate() should not be called if the channel can have its private browsing state obtained from its load group or load context.  Since the load group can't actually have a private browsing state except on the load group's load context, I'm assuming this means that it's supposed to be okay to call setPrivate() on a channel which has a load group but no associated load context (either via its notification callbacks or its load group's notification callbacks).

I am pretty sure http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h?rev=cc6f5b3ca9de&mark=1304-1304#1288 and http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h?rev=cc6f5b3ca9de#1217 agree with that; the only way to get a private browsing flag is by looking for a nsILoadContext.

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/PrivateBrowsingChannel.h?rev=cc6f5b3ca9de&mark=35-35,40-41#30 however asserts that the channel doesn't have a load group _at all_.  Even if that load group has no way of giving you a private browsing state.

Comment 1

4 years ago
Given that it looks like we allow consumers to call setPrivate and then set a loadgroup, we should probably make this consistent one way or the other. I can't think of any reason not to allow a loadgroup that doesn't provide nsILoadContext.
(In reply to comment #1)
> Given that it looks like we allow consumers to call setPrivate and then set a
> loadgroup, we should probably make this consistent one way or the other. I
> can't think of any reason not to allow a loadgroup that doesn't provide
> nsILoadContext.

I can't either... :/
(Assignee)

Comment 3

4 years ago
Created attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

This removes the checks for the load group; adds a load group in the unit test to make sure that's acceptable.  (It's still not _necessary_ for private channels, of course)
Assignee: nobody → mook.moz+mozbz
Attachment #757022 - Flags: review?(ehsan)
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

Review of attachment 757022 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I'd like Jason to look at it too.
Attachment #757022 - Flags: review?(jduell.mcbugs)
Attachment #757022 - Flags: review?(ehsan)
Attachment #757022 - Flags: review+
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

Review of attachment 757022 [details] [diff] [review]:
-----------------------------------------------------------------

Ehsan wrote this (horrible yet expedient) code, which I still shudder to look at.

So the existing code fails if there's a loadGroup.  The patch removes the check for the loadGroup entirely.  Don't we want to change it instead to still check if there's a loadGroup, but fail only if that loadGroup's callbacks provide nsILoadContext?   That would be my first guess, but I haven't thought about it too hard yet.

Comment 6

4 years ago
Yes, that is what I originally proposed.
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

Review of attachment 757022 [details] [diff] [review]:
-----------------------------------------------------------------

OK, so let's change this to still check if there's a loadGroup, but fail only if that loadGroup's callbacks provide nsILoadContext.
Attachment #757022 - Flags: review?(jduell.mcbugs) → review-
(Assignee)

Comment 8

4 years ago
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

Actually, this patch already checks the load group - NS_QueryNotificationCallbacks has that built-in so people don't forget to; pretty sure it's the reason the function exists :)

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h?rev=3c6f2394995d&mark=1240-1246#1217
Attachment #757022 - Flags: review- → review?(jduell.mcbugs)
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

Review of attachment 757022 [details] [diff] [review]:
-----------------------------------------------------------------

Oh right of course.
Attachment #757022 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 10

4 years ago
Comment on attachment 757022 [details] [diff] [review]
Relax checks to load context only; make sure to test

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a613c3c8a01
https://hg.mozilla.org/mozilla-central/rev/0a613c3c8a01
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.