Closed Bug 1411609 Opened 7 years ago Closed 5 years ago

Remove nsIChannel::Open/AsyncOpen


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






(Reporter: emk, Assigned: emk)


(Blocks 1 open bug)


(Whiteboard: [domsecurity-backlog1])


(1 file)

This is left only for addons that are no longer relevant.
Assignee: nobody → VYV03354
Summary: Remove asyncOpen from NetUtils.jsm → Remove nsIChannel::AsyncOpen
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?
Attachment #8923093 - Flags: review?(mcmanus)
Attachment #8923093 - Flags: review?(jduell.mcbugs)
Attachment #8923093 - Flags: review?(ckerschb)
this is so fundamental - can you check with mailnews to see if it would make their life difficult?
Flags: needinfo?(Pidgeot18)
Attachment #8923093 - Flags: review?(jduell.mcbugs)
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Redirecting to Jorg.
Flags: needinfo?(Pidgeot18) → needinfo?(jorgk)
I'll take a look on Monday.
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:
I little hard to see which call sites are nsIChannel::AsyncOpen and which are nsMsgProtocol::AsyncOpen.

There are a bunch of calls in JS:

The open calls are even harder to find. Hard to tell whether these are all the calls:

This query doesn't appear to yield more open calls:*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.
Flags: needinfo?(jorgk)
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
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).
Summary: Remove nsIChannel::AsyncOpen → Remove nsIChannel::Open/AsyncOpen
Depends on: 1452600
Probably worth a separate bug for mailnews.
(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.
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.
I duped bug 1279428 and bug 1279429 here becomes this seems more up to date?
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?
Flags: needinfo?(mcmanus)
Attachment #8923093 - Flags: review?(mcmanus)
Could you please somehow address our concern that we need to pass a context parameter, see bug 1452600 comment #8.
> 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?
Boris, I don't even understand your comment :-(

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

Closed: 5 years ago
Resolution: --- → DUPLICATE
No longer depends on: 1452600

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

Flags: needinfo?(mcmanus)
You need to log in before you can comment on or make changes to this bug.