1411609 - Remove nsIChannel::Open/AsyncOpen

Status: RESOLVED DUPLICATE of bug 1520868

(Core :: DOM: Security, enhancement, P3)

Description

[Masatoshi Kimura [:emk]]

This is left only for addons that are no longer relevant.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1411609 - Remove nsIChannel::AsyncOpen.

Review commit: https://reviewboard.mozilla.org/r/194294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194294/

Comment 2

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd36f8cbf9db0688f7ec98c11183b56bfdefea5d
Please ignore TV failures. It is bug 1406407.

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 8923093 [details]
Bug 1411609 - Remove nsIChannel::AsyncOpen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194294/diff/1-2/

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8923093 [details]
Bug 1411609 - Remove nsIChannel::AsyncOpen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194294/diff/2-3/

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8923093 [details]
Bug 1411609 - Remove nsIChannel::AsyncOpen.

I would be fine with removing asyncOpen()/open() but I suppose it's better that at Necko peer reviews that changeset. Jason, Pat, can you review or ask someone in your team?

Comment 6

[Patrick McManus [:mcmanus]]

this is so fundamental - can you check with mailnews to see if it would make their life difficult?

Comment 7

[Masatoshi Kimura [:emk]]

Redirecting to Jorg.

Comment 8

[Jorg K (CEST = GMT+2)]

I'll take a look on Monday.

Comment 9

[Jorg K (CEST = GMT+2)]

Masatoshi-san, I'm looking for the information you originally requested from Joshua Cranmer. I can't find it, so I assume you wanted to know whether we're fine with removing asyncOpen()/open().

I wanted to apply your patch to see how much C++ code is affected, but it doesn't apply any more, no wonder after more than five months.

Looking at DXR I see:
https://dxr.mozilla.org/comm-central/search?q=AsyncOpen(&redirect=false
I little hard to see which call sites are nsIChannel::AsyncOpen and which are nsMsgProtocol::AsyncOpen.

There are a bunch of calls in JS:
https://dxr.mozilla.org/comm-central/search?q=asyncOpen(&redirect=false

The open calls are even harder to find. Hard to tell whether these are all the calls:
https://dxr.mozilla.org/comm-central/search?q=channel.open(&redirect=false

This query doesn't appear to yield more open calls:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Aannel.*Open&redirect=false

So in short:
We can't stop you removing those functions. I can do a C++ patch once I can apply your patch and I can do a try run with the JS call sites also fixed.

Please let me know whether I need to answer anything else. I'm following bugmail, so no need for NI.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8923093 [details]
Bug 1411609 - Remove nsIChannel::AsyncOpen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194294/diff/3-4/

Comment 11

[Masatoshi Kimura [:emk]]

I updated the patch to tip. The easiest way to migrate is canging Open/AsyncOpen from XPCOM methods to internal methods.

nsIChannel::Open have no parameters from the JS perspective, so
https://dxr.mozilla.org/comm-central/search?q=.open()+-path%3Amozilla%2F+-path%3Aobj-x86_64-pc-linux-gnu%2F&redirect=false
will give a better result.

If I'm not missing, all c-c callers are using the system principal as the loading principal. So I think you can just replace open() with open2(). If the listener context (the second parameter of asyncOpen) is null or no listener methods use the listener context parameter, you can replace it asyncOpen2 either. If the listener context is not null and it is used in some listener methods, you will have to pass the context by other means (such as JS closures).

Comment 12

[Masatoshi Kimura [:emk]]

Probably worth a separate bug for mailnews.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> this is so fundamental - can you check with mailnews to see if it would make
> their life difficult?
Found the question, let's discuss this in bug 1452600.

Comment 14

[Jorg K (CEST = GMT+2)]

Well, the summary is (bug 1452600 comment #3) that we pass the URL being processed in the context parameter and unless I'm missing something, we'd have a hard time without AsyncOpen(). None of our code uses AsyncOpen2(), we merely implement it in the various protocols.

Comment 17

[Tom S]

I duped bug 1279428 and bug 1279429 here becomes this seems more up to date?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8923093 [details]
Bug 1411609 - Remove nsIChannel::AsyncOpen.

I am clearing the r? request since this patch most likely needs to be rebased anyway. Ultimately I would like to see that patch landed though. In a follow up we could rename AsyncOpen2() to AsyncOpen() to get somehow back to normal eventually.

Pat, does that sounds good? Would you be willing to r+ that change once it's rebased or are we missing something here?

Comment 19

[Jorg K (CEST = GMT+2)]

Could you please somehow address our concern that we need to pass a context parameter, see bug 1452600 comment #8.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Could you please somehow address our concern that we need to pass a context parameter

Add an interface that the relevant channels implement that allows setting a context and have the relevant callers QI to that interface and call the setter?

Comment 21

[Jorg K (CEST = GMT+2)]

Boris, I don't even understand your comment :-(

Comment 22

[Masatoshi Kimura [:emk]]

Bug 1520868 is doing something similar (with renaming Open2/AsyncOpen2 to Open/AsyncOpen).

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Clearing out my ni? queue with super old ni? requests which rendered unnecessary in the meantime.