786347 - Implementation of <a ping> does not follow the spec

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Tanvi Vyas[:tanvi]]

Looking at http://www.whatwg.org/specs/web-apps/current-work/#hyperlink-auditing, it looks like we should be sending Ping-To and sometimes Ping-From headers for pings.  Our implementation does follow the spec.

https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#250

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sure, our implementation way predates that spec.

It's also disabled by default, last I checked.

Comment 2

[Tanvi Vyas[:tanvi]]

Yes, I know.  But was advised to file a bug anyway :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Makes sense.  ;)

Comment 4

[Anne (:annevk)]

Given https://plus.google.com/+IlyaGrigorik/posts/fPJNzUf76Nx we should get on this I think.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch

I gave this a shot.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8346749 [details] [diff] [review]
0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch

So I don't quite understand what the spec is saying about Ping-To.  The same-origin case has different wording from the other two cases, but I can't tell what "the target of the hyperlink" is (it doesn't seem to be defined), so I'm not sure how "the address of the target of the hyperlink" differs from "the address of the absolute URL of the target of the hyperlink".  Could you please check with hixie as to what the spec means there?+    bool isHttps = 

>+bool isHttps = NS_FAILED(info->referrer->SchemeIs("https", &match)) || match;

I would rather this checked the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT flag.  That's what the mixed content blocker uses, say.

In practice right now that means https or about.  But this way it will keep working if we add support for another secure scheme.

And the boolean should probably be referrerIsSecure.

>+    if (!isHttps)
>+      httpChan->SetReferrer(info->referrer);

The condition there should be |if (!referrerIsSecure && !NS_SUCCEEDED(sameOrigin))|, no?

>+  NS_NAMED_LITERAL_CSTRING(uploadData, "PING");

This no longer contains the end-of-headers marker.

>   uploadChan->SetUploadStream(uploadStream, EmptyCString(), -1);

But this call is still telling the channel to treat the start of the stream as being in the headers.

What I think you want to do instead is to remove your SetRequestHeader() for Content-Type and instead pass NS_LITERAL_CSTRING("text/ping") here as the second argument.

And we need tests, ideally...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8346749 [details] [diff] [review]
0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch

Or rather, feedback+, I thought it was a review request for some reason...

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24086 is tracking the ping-to bit.

Comment 9

[Anne (:annevk)]

FWIW, I'm 100% sure Ping-To should contain the absolute URL of the relevant element. I do agree the specification could use clarification.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch (v2)

(In reply to Boris Zbarsky [:bz] from comment #6)
> So I don't quite understand what the spec is saying about Ping-To.  The
> same-origin case has different wording from the other two cases, but I can't
> tell what "the target of the hyperlink" is (it doesn't seem to be defined),
> so I'm not sure how "the address of the target of the hyperlink" differs
> from "the address of the absolute URL of the target of the hyperlink". 

Thanks for filing this, the spec has been fixed already.

> >+bool isHttps = NS_FAILED(info->referrer->SchemeIs("https", &match)) || match;
> 
> I would rather this checked the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT flag. 
> That's what the mixed content blocker uses, say.
> 
> In practice right now that means https or about.  But this way it will keep
> working if we add support for another secure scheme.

Good point, this will then still work should we implement TLS upgrades or something similar.

> >+    if (!isHttps)
> >+      httpChan->SetReferrer(info->referrer);
> 
> The condition there should be |if (!referrerIsSecure &&
> !NS_SUCCEEDED(sameOrigin))|, no?

Yes, thanks for catching that.

> What I think you want to do instead is to remove your SetRequestHeader() for
> Content-Type and instead pass NS_LITERAL_CSTRING("text/ping") here as the
> second argument.

Done.

> And we need tests, ideally...

Will work on those in a separate patch.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch (v3)

Using ExplicitSetUploadStream() instead of SetUploadStream() now because the latter overrides the HTTP method with "PUT" when a content type is given.

Extended SendPingInfo to include the actual target (href) of the link that is needed for the "Ping-To" header.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-786347-Tests-for-a-ping-implementation.patch

Tests.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Looks like I somehow introduced a leak here although I don't see anything obvious right now.

https://tbpl.mozilla.org/?tree=Try&rev=5fb96ea7fea2

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

AFAICT, to keep the request alive we explicitly call AddRef() on the loadGroup and set a timer to release it:

> // Prevent ping requests from stalling and never being garbage collected...
> nsCOMPtr<nsITimer> timer =
>     do_CreateInstance(NS_TIMER_CONTRACTID);
> if (timer) {
>   nsresult rv = timer->InitWithFuncCallback(OnPingTimeout, loadGroup,
>                                             PING_TIMEOUT,
>                                             nsITimer::TYPE_ONE_SHOT);
>   if (NS_SUCCEEDED(rv)) {
>     // When the timer expires, the callback function will release this
>     // reference to the loadgroup.
>     static_cast<nsILoadGroup *>(loadGroup.get())->AddRef();
>     loadGroup = 0;
>   }
> }

First, it looks like we should release load groups when the request has stopped and not after the timeout which is 10s by default. Second, the timeout never gets fired because nothing holds onto |timer| right?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch (v4)

Moved loadGroup and timer references to the nsPingListener. That fixes the leaks locally for me.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

The latest patch looks better:

https://tbpl.mozilla.org/?tree=Try&rev=96fb3738af19

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8347725 [details] [diff] [review]
0001-Bug-786347-Fix-a-ping-implementation-to-follow-spec.patch (v4)

Please cancel the timer in ~nsPingListener, so it won't get called on a dead loadgroup.

> nsPingListener::OnStartRequest(nsIRequest *request, nsISupports *context)

Setting up the timer here is way too late.  This will be called when the server has already responded...  You really do want to set the timer up right after you AsyncOpen or something.

r=me with those issues fixed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8347711 [details] [diff] [review]
0002-Bug-786347-Tests-for-a-ping-implementation.patch

Looks good, assuming Task.spawn() will not end up running SimpleTest.finish until all the subtasks have finished.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Please cancel the timer in ~nsPingListener, so it won't get called on a dead
> loadgroup.

Good point, moved that code to ~nsPingListener.

> > nsPingListener::OnStartRequest(nsIRequest *request, nsISupports *context)
> 
> Setting up the timer here is way too late.  This will be called when the
> server has already responded...  You really do want to set the timer up
> right after you AsyncOpen or something.

Right. The patch is now explicitly calling pingListener->StartTimeout() after AsyncOpen() and cancels the channel should setting up a timer fail.

> r=me with those issues fixed.

Thanks!

(In reply to Boris Zbarsky [:bz] from comment #18)
> Looks good, assuming Task.spawn() will not end up running SimpleTest.finish
> until all the subtasks have finished.

Yes, that's how it works. Task.spawn() takes a generator and the yields won't return until the promises passed to them have been resolved.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84c085a233e8
https://hg.mozilla.org/mozilla-central/rev/0099c1899f99

Comment 21

[Boris Zbarsky [:bzbarsky]]

>+  NS_IMETHODIMP StartTimeout();

Why?  This should just return nsresult, without messing with the calling convention, I'd think.

>+  mLoadGroup = 0;

No, this is bad.  We're not canceling the timer here, so now it potentially has a dangling pointer to the loadgroup....

>+    static_cast<nsIStreamListener*>(pingListener);

You shouldn't need the cast here.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-786347-Follow-up.patch

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #21)
> >+  mLoadGroup = 0;
> 
> No, this is bad.  We're not canceling the timer here, so now it potentially
> has a dangling pointer to the loadgroup....

So just for clarification: I do understand now that the nsPingListener doesn't go away immediately after OnStopRequest() was called and we thus need to cancel the timer.

Why do we need to also cancel the timer in ~nsPingListener? Can the nsPingListener just go away without OnStopRequest() being called? Is it more of a safety measure in case it's not being called?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Can the nsPingListener just go away without OnStopRequest() being called?

As the code is currently written, yes.  For example, say the AsyncOpen throws.  Since its return value is ignored and we press on to set up the timer anyway, it's possible that ~PingListener will get called when we return from SendPing but OnStopRequest won't get called at all.

If we bailed out on AsyncOpen failure, I believe we could in fact guarantee that OnStopRequest would happen before ~PingListener, as long as the channel is following the necko contract.  Which I wouldn't rely on too terribly much, since extensions can implement channels.  :(

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349098 [details] [diff] [review]
0003-Bug-786347-Follow-up.patch

You need to change the declaration of StartTimeout too, from NS_IMETHOD to just nsresult.

And probably better nullptr than 0 for the nulling things out bits.

r=me with that.  Thanks!

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Thanks for taking the time to explain!

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #24)
> You need to change the declaration of StartTimeout too, from NS_IMETHOD to
> just nsresult.
> 
> And probably better nullptr than 0 for the nulling things out bits.

Will do.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8939e7f497db

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8939e7f497db