Hey Honza, I am not entirely sure how aboutChache works, but I would suppose we should call asyncOpen2() on the underlying channel, right? Is it possible that the underlying channel does not have a loadInfo attached? If so, then potentially we should branch and have something like: nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo(); if (loadInfo && loadInfo->GetSecurityMode() != 0) { return mChannel->AsyncOpen2(aListener); } return mChannel->AsyncOpen2(aListener, nullptr); What do you think?

Comment on attachment 8765877 [details] [diff] [review] bug_1282750_asyncOpen2_aboutcache.patch Review of attachment 8765877 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/about/nsAboutCache.cpp @@ +143,5 @@ > // Kick the walk loop. > + nsresult rv = VisitNextStorage(); > + NS_ENSURE_SUCCESS(rv, rv); > + MOZ_ASSERT(!aContext, "asyncOpen2() does not take a context argument"); > + return mChannel->AsyncOpen2(aListener); please keep nsresult rv as the first and lonely declaration please keep the blank lines that separate logical blocks please don't use the NS_ENSURE_SUCCESS here, this is no critical code and there is no need to pollute the console. on the other hand, if you want to argue pro-NS_ENSURE, then why not also for the AsyncOpen call? @@ +148,5 @@ > } > > NS_IMETHODIMP nsAboutCache::Channel::AsyncOpen2(nsIStreamListener *aListener) > { > return AsyncOpen(aListener, nullptr); maybe rather implement this properly? (I mean - call AsyncOpen2 on mChannel)

also, when you are here, please make Open and Open2 just throw NOT_IMPLEMENTED. it's a foot gun anyway, it will deadlock main thread when called. thanks.

(In reply to Honza Bambas (:mayhemer) from comment #2) > > + return mChannel->AsyncOpen2(aListener); > > please keep nsresult rv as the first and lonely declaration > please keep the blank lines that separate logical blocks > please don't use the NS_ENSURE_SUCCESS here, this is no critical code and > there is no need to pollute the console. on the other hand, if you want to > argue pro-NS_ENSURE, then why not also for the AsyncOpen call? Sure, I don't have a strong opinion about that. > @@ +148,5 @@ > > } > > > > NS_IMETHODIMP nsAboutCache::Channel::AsyncOpen2(nsIStreamListener *aListener) > > { > > return AsyncOpen(aListener, nullptr); > > maybe rather implement this properly? (I mean - call AsyncOpen2 on mChannel) We use the same pattern everywhere, AsyncOpen2() calls asyncOpen() (even for nested channels like viewsourceChannel). I would like to keep it that way so we are consistent in what we are doing. (In reply to Honza Bambas (:mayhemer) from comment #3) > also, when you are here, please make Open and Open2 just throw > NOT_IMPLEMENTED. it's a foot gun anyway, it will deadlock main thread when > called. thanks. Sure thing!

Comment on attachment 8765895 [details] [diff] [review] bug_1282750_asyncOpen2_aboutcache.patch Review of attachment 8765895 [details] [diff] [review]: ----------------------------------------------------------------- thanks! I presume, one day we remove asyncOpen(1) altogether, right?

> thanks! I presume, one day we remove asyncOpen(1) altogether, right? Potentially: https://bugzilla.mozilla.org/show_bug.cgi?id=1279428 https://bugzilla.mozilla.org/show_bug.cgi?id=1279429

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/423f4de189ac Convert AboutCache to use AsyncOpen2() r=honza

Backed out for failing browser_aboutURLs.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a234db73110df9317a3d03fac6e58db83870e Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b2bda3d70086d111871f8c425cfaa5cb02bd3052 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30797622&repo=mozilla-inbound

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #8) > Backed out for failing browser_aboutURLs.js: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 2b0a234db73110df9317a3d03fac6e58db83870e > > Push with failure: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=b2bda3d70086d111871f8c425cfaa5cb02bd3052 > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=30797622&repo=mozilla- > inbound Sorry for the bustage and thanks for backing it out - I'll have a look.

Arrh, about:cache for a toplevel TYPE_DOCUMENT loads does not have the right security flags within the loadinfo yet, since we haven't converted docshell loads to use AsyncOpen2() as of now. I suppose we have to wait till Bug 1182569 is landed, see details for the load: doContentSecurityCheck { channelURI: about:cache loadingPrincipal: nullptr triggeringPrincipal: SystemPrincipal contentPolicyType: 6 securityMode: SEC_NORMAL initalSecurityChecksDone: no enforceSecurity: no }

Hey Honza, it seems when running browser_aboutURLs.js we are about to open a TYPE_DOCUMENT channel, but since we haven't converted docShell loads to use asyncOpen2() yet, we are hitting assertions within the contentSecurityManager. Recently I created a helper function that checks whether the channel can be openend using asyncOpen2() [1]. Instead of waiting for docshell loads to be converted we could use NS_MaybeOpenChannelUsingAsyncOpen2() as an interim solution. Can we agree on that? [1] https://hg.mozilla.org/integration/mozilla-inbound/rev/063d90be43fc#l3.47

Comment on attachment 8765895 [details] [diff] [review] bug_1282750_asyncOpen2_aboutcache.patch ># HG changeset patch ># User Christoph Kerschbaumer <ckerschb@christophkerschbaumer.com> ># Date 1467120740 -7200 ># Tue Jun 28 15:32:20 2016 +0200 ># Node ID 1d316fb5e5f72c7b322ca2e5d1f2f1aa6f644fb9 ># Parent 2ab57664bf7cafdd64e136e341527c275fc8c3aa >Bug 1282750 - Convert AboutCache to use AsyncOpen2() r=honza > >diff --git a/netwerk/protocol/about/nsAboutCache.cpp b/netwerk/protocol/about/nsAboutCache.cpp >--- a/netwerk/protocol/about/nsAboutCache.cpp >+++ b/netwerk/protocol/about/nsAboutCache.cpp >@@ -141,48 +141,36 @@ NS_IMETHODIMP nsAboutCache::Channel::Asy > if (!mChannel) { > return NS_ERROR_UNEXPECTED; > } > > // Kick the walk loop. > rv = VisitNextStorage(); > if (NS_FAILED(rv)) return rv; > >- rv = mChannel->AsyncOpen(aListener, aContext); >+ MOZ_ASSERT(!aContext, "asyncOpen2() does not take a context argument"); >+ rv = mChannel->AsyncOpen2(aListener); > if (NS_FAILED(rv)) return rv; > > return NS_OK; > } > > NS_IMETHODIMP nsAboutCache::Channel::AsyncOpen2(nsIStreamListener *aListener) > { > return AsyncOpen(aListener, nullptr); > } > > NS_IMETHODIMP nsAboutCache::Channel::Open(nsIInputStream * *_retval) > { >- nsresult rv; >- >- if (!mChannel) { >- return NS_ERROR_UNEXPECTED; >- } >- >- // Kick the walk loop. >- rv = VisitNextStorage(); >- if (NS_FAILED(rv)) return rv; >- >- rv = mChannel->Open(_retval); >- if (NS_FAILED(rv)) return rv; >- >- return NS_OK; >+ return NS_ERROR_NOT_IMPLEMENTED; > } > > NS_IMETHODIMP nsAboutCache::Channel::Open2(nsIInputStream * *_retval) > { >- return Open(_retval); >+ return NS_ERROR_NOT_IMPLEMENTED; > } > > nsresult > nsAboutCache::Channel::ParseURI(nsIURI * uri, nsACString & storage) > { > // > // about:cache[?storage=<storage-name>[&context=<context-key>]] > //

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4306097418bf Convert AboutCache to use AsyncOpen2() r=honza

Gijs pushed this to mozilla-release. https://hg.mozilla.org/releases/mozilla-release/rev/7356baae8e736a6c9444bdd21562df806a39766b