1529554 - Have nsIChannel return raw nsILoadinfo pointer within ::Loadinfo()

Status: NEW

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Currently we do the following within nsIChannel:

inline already_AddRefed<nsILoadInfo> LoadInfo()

which results in having callsites doing the following:

nsCOMPtr<nsILoadInfo> loadInfo = chan->LoadInfo();
loadInfo->Foo();

it would be nice if we could return a raw pointer so callsites can look like:
| chan->LoadInfo()->Foo();

which would also safe all the addref and deref quirks.

Problem is the following. If we update nsIChannel to do something like:

/**

• A C++-friendly version of loadInfo.
  */
  [noscript, notxpcom, nostdcall, binaryname(LoadInfo)]
  nsILoadInfo binaryLoadInfo();

then all the nsIChannel implementations have to implement that like:
| nsILoadInfo* FooChannel::LoadInfo() { return mLoadInfo; }

which is also not that great. I wonder if there are better solutions to that problem.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Baku, can you think of a better solution, or should we just have all nsIChannel implementations provide a ::LoadInfo() method?

Comment 2

[Andrea Marchesini [:baku]]

Well, in theory you could add this in nsIChannel.idl:

%{C++
inline nsILoadInfo* LoadInfo() {
nsCOMPtr<nsILoadInfo> result;
mozilla::DebugOnly<nsresult> rv = GetLoadInfo(getter_AddRefs(result));
MOZ_ASSERT(NS_SUCCEEDED(rv) || !result);
return result;
}
%}

But nothing guarantees the lifetime of the loadInfo in this way. If you add something like what you suggest, we can audit all the channel implementation and see if the loadInfo is set correctly.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrea Marchesini [:baku] from comment #2)

Well, in theory you could add this in nsIChannel.idl:

%{C++
inline nsILoadInfo* LoadInfo() {
nsCOMPtr<nsILoadInfo> result;
mozilla::DebugOnly<nsresult> rv = GetLoadInfo(getter_AddRefs(result));
MOZ_ASSERT(NS_SUCCEEDED(rv) || !result);
return result;
}
%}

But nothing guarantees the lifetime of the loadInfo in this way. If you add something like what you suggest, we can audit all the channel implementation and see if the loadInfo is set correctly.

Right, I thought about the solution you suggested above, but that still creates an intermediate nsCOMPtr - which I guess we want to avoid, right?

Would we accept a patch with my proposed solution from comment 0? Put differently, is this the way to go - or worth doing?

Comment 4

[Andrea Marchesini [:baku]]

Would we accept a patch with my proposed solution from comment 0? Put differently, is this the way to go - or worth doing?

Yes. It's fine to me.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1529554_loadinfo.patch

Basti, there is a WIP patch (I think it doesn't even compile) but it will give you an idea of what we are trying to accomplish.

Comment 6

[Sebastian Streich [:sstreich]]

Attachment: Bug 1529554 - Return Raw Pointer in LoadInfo() r=ckerschb

Comment 7

[Sebastian Streich [:sstreich]]

Currently, the Proposed Patch failes on tests that are using mock-Channels that are implemented on the JS side.

This is a side effect; as to be able to call chan->LoadInfo()->Foo(); like proposed in Comment 0, we must set the notxpcom flag (which implies noscript) on Loadinfo.

So this Patch would effectively remove support for js implementations of NsIChannels

My question is, do we have to support NsIchannels in js ?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Sebastian Streich [:sstreich] from comment #7)

My question is, do we have to support NsIchannels in js ?

Yes: https://searchfox.org/mozilla-central/search?q=nsI(Http)%3FChannel&case=true&regexp=true&path=*.js

Comment 9

[Sebastian Streich [:sstreich]]

Sorry i was unclear.
Its about whether we have to support JS to be able to implement and register new nsIChannels.
Which is afaik only used in a few unit-tests, if i'm correct.

This is not about accessing nsIChannels in js.
Sorry about my miscommunication.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Sebastian Streich [:sstreich] from comment #9)

Sorry i was unclear.
Its about whether we have to support JS to be able to implement and register new nsIChannels.
Which is afaik only used in a few unit-tests, if i'm correct.

This is not about accessing nsIChannels in js.
Sorry about my miscommunication.

I was wondering :) I think we only impl nsIChannel in tests, yes. I'd have to double check that, tho. I can't recall a particular test that would do this. This won't be that simple to find.

Are you sure we want to undergo all of this just to be able to type chan->LoadInfo()->Foo(); on few places in cpp?

What about a virtual method defined by default as baku suggest in comment 2 that would be overridden by some heavily used classes like nsHttpChannel to return mLoadInfo directly? (With a single thread access assertions, preferably).

Comment 11

[Honza Bambas (:mayhemer)]

(valentin is on pto)

Comment 12

[Sebastian Streich [:sstreich]]

Attachment: Bug 1529554 - Remove GetLoadInfo callsites r=ckerschb

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Honza, being able to write chan->LoadInfo()->Foo() would be a nice to have, but maybe not for every price. Maybe we should simply update nsIChannel.idl [1] and replace

inline already_AddRefed<nsILoadInfo> LoadInfo()
with
inline nsILoadInfo* LoadInfo()

That way, we would still have the add/deref overhead but we could write chan->LoadInfo()->Foo().

Alternatively, we should just let it go and mark this one as a wontfix. Updating and investing that much time to eliminate JS implemented nsIChannels sounds like overkill after all.

Any preferences?

[1] https://searchfox.org/mozilla-central/source/netwerk/base/nsIChannel.idl#358

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)

Honza, being able to write chan->LoadInfo()->Foo() would be a nice to have, but maybe not for every price. Maybe we should simply update nsIChannel.idl [1] and replace

inline already_AddRefed<nsILoadInfo> LoadInfo()
with
inline nsILoadInfo* LoadInfo()

yes. and if the method is also made virtual, some channels may override it and get rid of the addref/release when mLoadInfo can be returned directly. single thread access assertions should be added too.

Comment 15

[Sebastian Streich [:sstreich]]

Attachment: Bug 1529554 - Return Raw Pointer in LoadInfo() r=ckerschb

Comment 16

[Sebastian Streich [:sstreich]]

The other Attempt using the Virt-Method had a few side effects, where the effort to make that one work is bigger than the win in being able to do chan->LoadInfo()->Foo().

As far as i can tell those are the Tests that are still using JS-Channel implementations and thus blocking my intial attempt implementing Comment 0 :

netwerk/test/unit/test_bug365133.js
netwerk/test/unit/test_bug484684.js
netwerk/test/unit/test_bug515583.js
netwerk/test/unit/test_bug543805.js
netwerk/test/unit/test_bug894586.js
dom/base/test/chrome/test_bug682305.html

Most of those are actually related to the FTP-Channel,
so i would propose we might reassess this again later.

Comment 17

[Phabricator Automation]

Comment on attachment 9058506 [details]
Bug 1529554 - Remove GetLoadInfo callsites r=ckerschb

Revision D27647 was moved to bug 1546913. Setting attachment 9058506 [details] to obsolete.