Convert AboutCache to use AsyncOpen2()

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Assignee: nobody → ckerschb
Blocks: 1182535
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
Summary: Convert ABoutCache to use AsyncOpen2() → Convert AboutCache to use AsyncOpen2()
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?
Attachment #8765877 - Flags: review?(honzab.moz)
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)
Attachment #8765877 - Flags: review?(honzab.moz)
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!
Attachment #8765895 - Flags: review?(honzab.moz)
Attachment #8765877 - Attachment is obsolete: true
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?
Attachment #8765895 - Flags: review?(honzab.moz) → review+
> 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
(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.
Flags: needinfo?(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
}
Depends on: 1182569
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
Attachment #8766319 - Flags: review?(honzab.moz)
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>]]
>     //
Attachment #8765895 - Attachment is obsolete: true
Attachment #8766319 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/4306097418bf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
See Also: → 1318664
You need to log in before you can comment on or make changes to this bug.