1282750 - Convert AboutCache to use AsyncOpen2()

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1282750_asyncOpen2_aboutcache.patch

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 2

[Honza Bambas (:mayhemer)]

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)

Comment 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1282750_asyncOpen2_aboutcache.patch

(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 5

[Honza Bambas (:mayhemer)]

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?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

> 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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/423f4de189ac
Convert AboutCache to use AsyncOpen2() r=honza

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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
}

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1282750_asyncOpen2_aboutcache.patch

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 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8765895 [details] [diff] [review]
bug_1282750_asyncOpen2_aboutcache.patch

># HG changeset patch
># User Christoph Kerschbaumer <[e-mail]>
># 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>]]
>     //

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4306097418bf
Convert AboutCache to use AsyncOpen2() r=honza

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4306097418bf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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