1193924 - Add assertion to AsyncOpen to make sure AsyncOpen(2) was called first when securityflags in loadInfo are set

Status: RESOLVED FIXED

Description

[Christoph Kerschbaumer [:ckerschb]]

We should land assertions to make sure that asyncOpen(2) is always called before asyncOpen(). We can make sure that happenened by investigating the 'securtyMode' within the Loadinfo to determine if asyncOpen(2) should have been called. If so, we can check that 'initialSecurityChecksDone' flag is set.

I think it's particularly relevant now that we are getting into callsites where channel creation and opening happens at different places.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Created attachment 8647121 [details] [diff] [review]
bug_1182569_asyncopen2_assertions.patch

Do you really have to assign an already_addrefed value to a temprorary first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do I really have to assign the loadInfo to a tmp variable? It seems, all over the codebase, this is what we do :-(

Also, I think we may want to also check for mLoadInfo first, because if an addon calls newChannel instead of newChannel2, then the channel does not have a loadInfo. So the assertion looks something like this:

> MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
>            mLoadInfo->GetInitialSecurityCheckDone(),
>             "security flags in loadInfo but asyncOpen(2) not called");

What do you think Jonas? Do we still care about the case where the channel might not have a loadInfo??

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Do you really have to assign an already_addrefed value to a temprorary
> first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do
> I really have to assign the loadInfo to a tmp variable? It seems, all over
> the codebase, this is what we do :-(

If you didn't assign to a temporary, you'd leak. So yes, you have to assign an already_AddRefed before you can use it.

But I don't know why you are use

    nsCOMPtr<nsILoadInfo> loadInfo;
    GetLoadInfo(getter_AddRefs(loadInfo));

rather than

    nsCOMPtr<nsILoadInfo> loadInfo = GetLoadInfo();

?

> Also, I think we may want to also check for mLoadInfo first, because if an
> addon calls newChannel instead of newChannel2, then the channel does not
> have a loadInfo. So the assertion looks something like this:
> 
> > MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
> >            mLoadInfo->GetInitialSecurityCheckDone(),
> >             "security flags in loadInfo but asyncOpen(2) not called");
> 
> What do you think Jonas? Do we still care about the case where the channel
> might not have a loadInfo??

Yes. We still care about addons creating channels. I don't know why anything would have changed here?

However, your code above gets the nullcheck wrong. What you want is

MOZ_ASSERT(!mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
           mLoadInfo->GetInitialSecurityCheckDone(),
            "security flags in loadInfo but asyncOpen(2) not called");

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Created attachment 8647307 [details] [diff] [review]
bug_1193924_asyncopen_assertions.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> > What do you think Jonas? Do we still care about the case where the channel
> > might not have a loadInfo??
> 
> Yes. We still care about addons creating channels. I don't know why anything
> would have changed here?

I updated the assertion to account for a null loadInfo. What I meant is, do we still care about addon code for debug builds, because in release builds they would work just fine. Anyway, I think it makes the most sense to make the assertion account for a null loadInfo.


Further, there are 2 cases where we have to use a slightly different assertion, namely in
* nsJARChannel::AsyncOpen
* nsViewSourceChannel::AsyncOpen

In those two cases the outer channel does not perform security checks in AsyncOpen(2), but rather sets the 'EnforceSecurityFlag'. I think it still makes sense to assert on that in those two cases if the securityMode is set in the LoadInfo.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8647307 [details] [diff] [review]
bug_1193924_asyncopen_assertions.patch

Review of attachment 8647307 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +569,5 @@
> +    {
> +    nsCOMPtr<nsILoadInfo> loadInfo = nsIChannel::GetLoadInfo();
> +    MOZ_ASSERT(!loadInfo || loadInfo->GetSecurityMode() == 0 ||
> +               loadInfo->GetInitialSecurityCheckDone(),
> +               "security flags in loadInfo but asyncOpen(2) not called");

Err.. all of these should say asyncOpen2, right? asyncOpen was called.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b27e40987b982c17fa8271ad94754228f4e92
changeset:  f99b27e40987b982c17fa8271ad94754228f4e92
user:       Christoph Kerschbaumer <[e-mail]>
date:       Wed Aug 12 21:36:33 2015 -0700
description:
Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G emulator bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=12829829&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ad1b01b299

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Make mLoadInfo protected rather than private

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57dc2cd0ff0

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/276b3c44eff17b097d3cdc3c1f769597d2aaf62e
changeset:  276b3c44eff17b097d3cdc3c1f769597d2aaf62e
user:       Christoph Kerschbaumer <[e-mail]>
date:       Wed Aug 12 21:36:33 2015 -0700
description:
Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/276b3c44eff1