If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use channel->ascynOpen2 for PING in docshell/base/nsDocShell.cpp

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Comment 1

2 years ago
Created attachment 8645896 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch
Attachment #8645896 - Flags: review?(jonas)
Comment on attachment 8645896 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

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

It'd also be awesome if you want to do the same cleanup that we did for the sendBeacon code. I.e. create a loadgroup and use the docshell as the notification callback on that loadgroup. That way you can remove the nsINetworkInterceptController hack that's there now. Up to you.

::: docshell/base/nsDocShell.cpp
@@ +328,5 @@
>    // Ignore non-HTTP(S)
> +  bool isHttpScheme =
> +    (NS_SUCCEEDED(aURI->SchemeIs("http", &isHttpScheme)) && isHttpScheme) ||
> +    (NS_SUCCEEDED(aURI->SchemeIs("https", &isHttpScheme)) && isHttpScheme);
> +  return isHttpScheme;

Ideally I'd change this to check that we're not pinging data: to keep things consistent with sendBeacon, but I'll leave that to you.

@@ +534,5 @@
>  {
>    nsCOMPtr<nsIURI> newURI;
>    aNewChan->GetURI(getter_AddRefs(newURI));
>  
> +  if (!CheckPingURI(newURI)) {

Remove this check. The whole thing of only allowing http URLs is somewhat unneccessary, so we should only bother doing it on the original URL IMO.

This'll mean that you'll end up with just one callsite of CheckPingURI, so you can just inline the function at that callsite.

@@ +591,5 @@
>    nsCOMPtr<nsIChannel> chan;
>    NS_NewChannel(getter_AddRefs(chan),
>                  aURI,
>                  doc,
> +                nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

There's some code in the redirect handler which forces the ping to be same-origin when "info->requireSameHost" is true.

Rather than enforcing that in the redirect handler, use SAME_ORIGIN here if that's true and remove the current code to handle the same-origin restriction.

I think that means that you can remove the whole redirect-handler. So the nsPingListener won't need to implement nsIChannelEventSink.
Attachment #8645896 - Flags: review?(jonas) → review-
(Assignee)

Comment 3

2 years ago
Created attachment 8646028 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> It'd also be awesome if you want to do the same cleanup that we did for the
> sendBeacon code.

That cleans things up quite drastically. Looks way better this way - thanks for all the feedback!
Attachment #8645896 - Attachment is obsolete: true
Attachment #8646028 - Flags: review?(jonas)
Comment on attachment 8646028 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

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

::: docshell/base/nsDocShell.cpp
@@ +426,1 @@
>    nsCOMPtr<nsIContent> mContent;

You can remove mContent as well.

@@ +510,4 @@
>      // only require the same hostname.  The scheme and port may differ.
>      if (!IsSameHost(aURI, info->referrer)) {
>        return;
>      }

You can remove this check since the channel will take care of it. Which means that you can kill the IsSameHost function entirely.

@@ +517,5 @@
>  
> +  nsSecurityFlags securityFlags =
> +    info->requireSameHost ?
> +    nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED :
> +    nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;

Nit: I would generally lean towards inlining the flags computation inside the NS_NewChannel statement. Just to make sure that the code to create the channel is kept with the code to compute the flags.

It's a major aspect of the security policy so it's good that we can always tell with certainty what policy is used for various parts of the code.

Up to you though. Especially since it might be hard to keep the 80-column limit if you inline.

@@ +629,5 @@
>  
> +  nsRefPtr<nsPingListener> pingListener =
> +    new nsPingListener(aContent, loadGroup);
> +
> +  chan->AsyncOpen2(pingListener);

You should set the loadgroup in the listener right here, if and only if AsyncOpen2 succeeded? Otherwise you'll leak if the channel fails to open. This was a problem in the old code too, but is a bigger problem now since opening the channel can fail in more ways.

Making mLoadGroup public is the easiest way to do so.
Attachment #8646028 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8646455 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

(In reply to Jonas Sicking (:sicking) from comment #4)

Thanks Jonas - incorporated all of your other suggestions. Carrying over r+.
Attachment #8646028 - Attachment is obsolete: true
Attachment #8646455 - Flags: review+
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3cbc5af05a
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=213e08d4a159
(Assignee)

Comment 8

2 years ago
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/fa170734154a12789670d9621be3483105b35558
changeset:  fa170734154a12789670d9621be3483105b35558
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Thu Aug 13 08:53:28 2015 -0700
description:
Bug 1192955 - Use channel->ascynOpen2 for PING in docshell/base/nsDocShell.cpp (r=sicking)
https://hg.mozilla.org/mozilla-central/rev/fa170734154a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Updated

2 years ago
Depends on: 1257730
You need to log in before you can comment on or make changes to this bug.