Closed Bug 1214122 Opened 9 years ago Closed 9 years ago

Do not wrap addon channels if they use a gecko internal channel implementation

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 2 obsolete files)

Separating this issue from bug 1206199. A custom protocol handle may return a normal gecko channel. We do not need to wrap that channel at all.
Attached patch bug_1214122.patch (obsolete) — Splinter Review
I moved this patch to a separate bug because t is easier for qa. Honza can you please review it. there are some comments: https://bugzilla.mozilla.org/show_bug.cgi?id=1206199#c61
Flags: needinfo?(honzab.moz)
It is worth checking whether we need a wrapper at all. Maybe all custom protocolHandler actually return a gecko channel.
Dragana, I am assigning this bug to you because you already did all the work. The other question is, now since we are having this bug, do you think we don't even need the patch from Bug 1206199 for mediating AsyncOnChannelRedirect?
Assignee: nobody → dd.mozilla
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(honzab.moz)
Attachment #8672968 - Flags: review?(honzab.moz)
Comment on attachment 8672968 [details] [diff] [review] bug_1214122.patch Review of attachment 8672968 [details] [diff] [review]: ----------------------------------------------------------------- I'll quickly check the final version. ::: netwerk/base/nsIOService.cpp @@ +762,5 @@ > + > + // The protocol handler does not implement NewProxiedChannel2, so > + // we need to wrap the channel. It is also possible that the custom > + // protocol handler has actually returned a gecko httpChannel > + // so we need to check for that option as well. ...option as well. and then do what with that? (The comment is kinda incomplete. You could also simply refer to a comment for the MaybeWrap function as an explanation.) ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +98,5 @@ > + if (isGeckoChannel) { > + // If it is a gecko channel (ftp or http) we do not need to wrap it. > + channel->SetLoadInfo(aLoadInfo); > + } else { > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInt = I think you should check nsIHttpChannel interface, no? Add-ons have no obligation to impl the nsIHttpChannelInternal interface.
Attachment #8672968 - Flags: review?(honzab.moz) → feedback+
Attached patch bug_1214122.patch (obsolete) — Splinter Review
Attachment #8672968 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8673754 - Flags: review?(honzab.moz)
Comment on attachment 8673754 [details] [diff] [review] bug_1214122.patch Review of attachment 8673754 [details] [diff] [review]: ----------------------------------------------------------------- Disclaimer: not locally tested and not checked in any way this actually fixes the original issue(s). ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +83,5 @@ > CHANNELWRAPPERLOG(("nsSecCheckWrapChannel::nsSecCheckWrapChannel [%p] (%s)",this, spec.get())); > } > } > > +//static // static
Attachment #8673754 - Flags: review?(honzab.moz) → review+
Attachment #8673754 - Attachment is obsolete: true
Attachment #8674853 - Flags: review+
Keywords: checkin-needed
No longer blocks: 1206199
Summary: Check whether a custom protocolHandler actually returns a gecko channel → Do not wrap addon channels if they use a gecko internal channel implementation
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8674853 [details] [diff] [review] bug_1214122.patch Approval Request Comment [Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1120487 [User impact if declined]: Users installing addons that implement their own protocolHandlers will crash the browser. [Describe test coverage new/current, TreeHerder]: tested with http://addons.computerbild.de/antiabzocke/plugin/COMPUTERBILD-Abzockschutz-1.0.59.xpi [Risks and why]: Low, because the patch only applies to users that install addons which implement their own protocol handlers (on AMO we where able to find ~150 such addons). [String/UUID change made/needed]: yes
Attachment #8674853 - Flags: approval-mozilla-beta?
Attachment #8674853 - Flags: approval-mozilla-aurora?
Dragana, how can we test that? Thanks
Flags: qe-verify+
Flags: needinfo?(dd.mozilla)
Flags: a11y-review?
(just use the xpi mentioned in comment #12?)
Comment on attachment 8674853 [details] [diff] [review] bug_1214122.patch Fix a crash, taking it even if I am not really happy with the size of the patch.
Attachment #8674853 - Flags: approval-mozilla-beta?
Attachment #8674853 - Flags: approval-mozilla-beta+
Attachment #8674853 - Flags: approval-mozilla-aurora?
Attachment #8674853 - Flags: approval-mozilla-aurora+
N-i to make sure Jorge sees the idl change (we will crash otherwise)
Flags: needinfo?(jorge)
(In reply to Sylvestre Ledru [:sylvestre] from comment #13) > Dragana, how can we test that? Thanks This is only an intermediate bug, which avoid wrapping a channel if it is not really needed. This fix part of the crashes. Bug 1214786 will fix all crashes with this addon. Sorry for fixing it one by one.
Flags: needinfo?(dd.mozilla)
a11y-review flag seems to have been set unintentionally. Don't see anything accessibility-related here that needs verification from our team.
Flags: a11y-review?
(In reply to Dragana Damjanovic [:dragana] from comment #18) > (In reply to Sylvestre Ledru [:sylvestre] from comment #13) > > Dragana, how can we test that? Thanks > > This is only an intermediate bug, which avoid wrapping a channel if it is > not really needed. > This fix part of the crashes. > Bug 1214786 will fix all crashes with this addon. > Sorry for fixing it one by one. Well, except if you can get the patch landed today, this is going probably going to be too late. Today, we are going to build 42 beta 9. That means that only RC is the only remaining build. So, should we backout this patch or does it make sense to keep it?
Flags: needinfo?(dd.mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #21) > (In reply to Dragana Damjanovic [:dragana] from comment #18) > > (In reply to Sylvestre Ledru [:sylvestre] from comment #13) > > > Dragana, how can we test that? Thanks > > > > This is only an intermediate bug, which avoid wrapping a channel if it is > > not really needed. > > This fix part of the crashes. > > Bug 1214786 will fix all crashes with this addon. > > Sorry for fixing it one by one. > Well, except if you can get the patch landed today, this is going probably > going to be too late. Today, we are going to build 42 beta 9. That means > that only RC is the only remaining build. > > So, should we backout this patch or does it make sense to keep it? I am only helping ckerschb, so I haven't look at the deadlines. We should keep this patch it prevents addon from crashing, just a small part of the addon doesn't work. (I am starting to mix up this bugs) I will see to get the other one landed today.
Flags: needinfo?(dd.mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #17) > N-i to make sure Jorge sees the idl change (we will crash otherwise) Thanks, doesn't appear to be used by add-ons.
Flags: needinfo?(jorge)
(In reply to Dragana Damjanovic [:dragana] from comment #18) > This is only an intermediate bug, which avoid wrapping a channel if it is > not really needed. > This fix part of the crashes. > Bug 1214786 will fix all crashes with this addon. > Sorry for fixing it one by one. I do not clearly understand whether this bug can be manually tested or not. Dragana, if this issue needs to be verified, could you please provide some reliable steps in order to test it. Otherwise, please remove the qe-verify flag.
Flags: needinfo?(dd.mozilla)
Depends on: 1218143
Depends on: 1195520
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #24) > (In reply to Dragana Damjanovic [:dragana] from comment #18) > > > This is only an intermediate bug, which avoid wrapping a channel if it is > > not really needed. > > This fix part of the crashes. > > Bug 1214786 will fix all crashes with this addon. > > Sorry for fixing it one by one. > > I do not clearly understand whether this bug can be manually tested or not. > > Dragana, if this issue needs to be verified, could you please provide some > reliable steps in order to test it. Otherwise, please remove the qe-verify > flag. This will be verified together with bug 1214786.
Flags: needinfo?(dd.mozilla)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: