1271921 - Log Deprecation within ioService when calling NewChannel2() fails

Status: RESOLVED INVALID

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1271921_log_deprecation_warning_within_ioservice.patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8751210 [details] [diff] [review]
bug_1271921_log_deprecation_warning_within_ioservice.patch

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

Actually, I don't think we should do this. I'm not convinced that I prefer addons implementing asyncOpen2 themselves. It seems quite easy for them to forget to do the proper security checks.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Actually, I don't think we should do this. I'm not convinced that I prefer
> addons implementing asyncOpen2 themselves. It seems quite easy for them to
> forget to do the proper security checks.

Well, at the moment we fall back to using AsyncOpen() in case AsyncOpen2() is not implemented. AsyncOpen() doesn't do any security checks at all, so no difference in my opinion. The reason why I would like to log a warning is, because we could really deprecate asyncOpen completely and move the logic from asyncOpen into asyncOpen2 at some point.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Well, at the moment we fall back to using AsyncOpen() in case AsyncOpen2() is not implemented.

No we don't. We implement AsyncOpen2 ourselves when newChannel2 is not implemented. No?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> > Well, at the moment we fall back to using AsyncOpen() in case AsyncOpen2() is not implemented.
> 
> No we don't. We implement AsyncOpen2 ourselves when newChannel2 is not
> implemented. No?

Arrh, sorry for the confusion. Not AsyncOpen!!!
s/asyncOpen/newchannel in comment 3.

In case NewChannel2() is not implemented, we fall back to using NewChannel().

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

You confused me within comment 2. The deprecation warning is only about NewChannel.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When newChannel2 is not implemented, we don't fall back to using newChannel. We use newChannel but we *also* create a wrapper which properly implements AsyncOpen2. I think that's better than asking addon developers to implement AsyncOpen2 themselves.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Oh, sure, you are right, that is indeed the better solution. But that also means we can never really deprecate newChannel.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We can deprecate IOService.newChannel. But we couldn't deprecate nsIProtocolHandler.newChannel indeed. But only nsIOService.cpp should be calling nsIProtocolHandler.newChannel.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> We can deprecate IOService.newChannel. But we couldn't deprecate
> nsIProtocolHandler.newChannel indeed. But only nsIOService.cpp should be
> calling nsIProtocolHandler.newChannel.

Sure, agreed. I am just worried if we don't clean up everything our code gets messy over time.