Last Comment Bug 319368 - Implement <a ping>
: Implement <a ping>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Linux
: -- enhancement (vote)
: mozilla1.9alpha1
Assigned To: Darin Fisher
:
: Andrew Overholt [:overholt]
Mentors:
http://whatwg.org/specs/web-apps/curr...
Depends on: 323793 323924 323959 324645
Blocks: 323238
  Show dependency treegraph
 
Reported: 2005-12-06 15:52 PST by Hixie (not reading bugmail)
Modified: 2010-07-16 11:21 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (8.70 KB, patch)
2005-12-06 18:33 PST, Darin Fisher
no flags Details | Diff | Splinter Review
v1.1 patch (14.20 KB, patch)
2005-12-08 12:43 PST, Darin Fisher
no flags Details | Diff | Splinter Review
v1.2 patch (16.11 KB, patch)
2005-12-08 14:03 PST, Darin Fisher
bzbarsky: superreview+
Details | Diff | Splinter Review
v1.3 patch (19.79 KB, patch)
2005-12-09 13:51 PST, Darin Fisher
jst: review-
Details | Diff | Splinter Review
v1.4 patch (24.32 KB, patch)
2006-01-10 17:03 PST, Darin Fisher
jst: review+
jst: approval‑branch‑1.8.1-
Details | Diff | Splinter Review
patch to disable <a ping> in tbird [fixed-on-trunk] (1.28 KB, patch)
2006-01-11 08:01 PST, Darin Fisher
mscott: review+
Details | Diff | Splinter Review
Disable in SeaMonkey for the moment (Checked in trunk) (1.18 KB, patch)
2006-01-12 15:18 PST, Ian Neal
neil: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Hixie (not reading bugmail) 2005-12-06 15:52:05 PST
We should try and do an experimental implementation of <a ping>, to see if there are any unexpected real-world problems.

Spec: http://whatwg.org/specs/web-apps/current-work/#ping

I can make some test cases if you want.
Comment 1 Darin Fisher 2005-12-06 18:33:47 PST
Created attachment 205185 [details] [diff] [review]
v1 patch
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-07 16:20:52 PST
+  nsCOMPtr<nsIStreamListener> listener = new nsPingListener();

this doesn't handle OOM

I wonder if something like DiscardData should be in nsStreamUtils.h... such a function is used often.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 16:55:53 PST
So this sends pings for the following cases:

1)  HTML form submits
2)  <link> elements if users click on them
3)  XLinks
4)  <area> elements when the user clicks an imagemap
5)  The Navigate() method in the ActiveX control
6)  <isindex> searches
7)  nsPluginInstanceOwner::GetURL calls

Most of these look very underable to me (e.g. for XLinks the "ping" attribute in the per-element namespace partition could be anything, since the node could be in any XML dialect in that case).
Comment 4 Hixie (not reading bugmail) 2005-12-07 17:13:34 PST
IMHO it should only fire for user and scripted activations of <a> elements and <area> elements (<area> is not yet defined in the spec).
Comment 5 Darin Fisher 2005-12-07 21:56:17 PST
> +  nsCOMPtr<nsIStreamListener> listener = new nsPingListener();
> 
> this doesn't handle OOM

Yup, good catch.  I'll fix that.


> I wonder if something like DiscardData should be in nsStreamUtils.h... such a
> function is used often.

Yeah, that's a good idea.  I think there may even be one other use of the same type of function in the codebase elsewhere.


Re: node name checking.  That was an oversight on my part.  I wonder if I should include ping support for XHTML namespace(s).  Thoughts?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 22:14:03 PST
Seems OK to me if Ian doesn't see any issues with it...
Comment 7 Hixie (not reading bugmail) 2005-12-08 11:30:40 PST
In the new world, HTML elements and XHTML elementss are both in the XHTML namespace. Mozilla doesn't implement that quite yet but it is coming, in theory.

Which is to say, yes, it should work for text/html <a> and <xhtml:a> (but not any other elements).
Comment 8 Darin Fisher 2005-12-08 12:43:42 PST
Created attachment 205323 [details] [diff] [review]
v1.1 patch

In my local tree, I have added NS_DiscardSegment to xpcom/build/dlldeps.cpp as well.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2005-12-08 12:48:47 PST
That allows <a> elements in the null namespace, which is wrong in general... (they could be non-HTML).

Since we're just checking for HTML, I'd use IsContentOfType(nsIContent::eHTML) (and then check the tag).
Comment 10 Darin Fisher 2005-12-08 12:58:33 PST
> Since we're just checking for HTML, I'd use IsContentOfType(nsIContent::eHTML)
> (and then check the tag).

Oh, that's an awesome little function.  Thanks!
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2005-12-08 13:01:37 PST
Yeah, working without IsContentOfType is hard.... ;)
Comment 12 Darin Fisher 2005-12-08 14:03:28 PST
Created attachment 205335 [details] [diff] [review]
v1.2 patch

Boris: you willing to SR this?
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2005-12-08 14:22:04 PST
Comment on attachment 205335 [details] [diff] [review]
v1.2 patch

>Index: docshell/base/nsWebShell.cpp
>+  nsIAtom *nameAtom = content->NodeInfo()->NameAtom();

nsIAtom *nameAtom = content->Tag();

sr=bzbarsky with that nit
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-08 23:28:34 PST
Comment on attachment 205335 [details] [diff] [review]
v1.2 patch

+  nsIAtom *nameAtom = content->NodeInfo()->NameAtom();

maybe Tag(), although they both end up being the same

+  nsString value;
+  content->GetAttr(kNameSpaceID_None, pingAtom, value);

why not an autostring? you suspect that this will usually be longer than 64 chars?

you could replace http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#51 as well

and http://lxr.mozilla.org/seamonkey/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#790 it seems
Comment 15 Darin Fisher 2005-12-09 00:32:48 PST
> maybe Tag(), although they both end up being the same

Yup, boris made the same suggestion.


> +  nsString value;
> +  content->GetAttr(kNameSpaceID_None, pingAtom, value);
> 
> why not an autostring? you suspect that this will usually be longer than 64
> chars?

Because I know that GetAttr returns a ref-counted buffer ;-)


> you could replace
> http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#51
> as well
> 
> and
> http://lxr.mozilla.org/seamonkey/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#790
> it seems

Will do, thanks.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2005-12-09 09:15:20 PST
> Because I know that GetAttr returns a ref-counted buffer ;-)

You sure?  It does for now, but we've been considering switching to atoms for short attr values...
Comment 17 Darin Fisher 2005-12-09 13:39:29 PST
> You sure?  It does for now, but we've been considering switching to atoms for
> short attr values...

Do you think that I should change this code to use nsAutoString for the sake of API encapsulation?
Comment 18 Darin Fisher 2005-12-09 13:51:19 PST
Created attachment 205421 [details] [diff] [review]
v1.3 patch

With Tag() and more reuse of NS_DiscardSegment.
Comment 19 Darin Fisher 2005-12-09 13:51:44 PST
I also switched that nsString to a nsAutoString.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2005-12-09 15:46:42 PST
I think it's better to go with nsAutoString, yeah....
Comment 21 Darin Fisher 2005-12-15 18:31:17 PST
Comment on attachment 205421 [details] [diff] [review]
v1.3 patch

moving review request to jst.  biesi: any additional feedback from you is still most welcome! ;-)
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-16 09:34:26 PST
Comment on attachment 205421 [details] [diff] [review]
v1.3 patch

From the spec:

  User agents should allow the user to adjust this behaviour, for example in
  conjunction with a setting that disables the sending of HTTP Referrer headers.

Sounds like we really *should* do that. I for one would rather not have this enabled, given the slow high-latency internet connection I frequently use, not to mention the obvious privacy concerns with this.

Add a pref check or something that one can use to disable this and I'll review this, other than that this change looks good.
Comment 23 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-17 13:36:53 PST
Comment on attachment 205421 [details] [diff] [review]
v1.3 patch

nsWebShell.cpp:

You are using the wrong base URI... each node can have its own base URI, so you should use content->GetBaseURI() (xml:base allows that, and apparently we also have a _baseHref attribute for such a thing)

This also seems to potentially leave leading spaces in the uri string passed to ios->NewChannel, does that work?

r=biesi with that fixed
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-17 13:37:57 PST
wait, one more thing... if I have ping=" " (i.e. a single space as attribute value), wouldn't this try to create a URI for "", which is identical to the base URI, which will in the normal case request the document itself?
Comment 25 Darin Fisher 2005-12-17 13:45:10 PST
> wait, one more thing... if I have ping=" " (i.e. a single space as attribute
> value), wouldn't this try to create a URI for "", which is identical to the
> base URI, which will in the normal case request the document itself?

good observation.

another thing I thought about:

since these loads are not added to any loadgroup, there is nothing the user can do to cancel them.  that is both part of the design of <a ping> as well as a potential problem.  at the very least, i think we should probably set a timeout on how long we are willing to wait for a ping request to complete.

jst: yeah, adding a pref is a good idea, but what privacy concerns are you in particular thinking about?  if you are talking about URL loads going out to places the user didn't know about, how is that really that much different from JS intercepting a link click and navigating the browser elsewhere before redirecting the user to the place where they thought they were going?
Comment 26 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-17 14:56:00 PST
(that JS could also use XMLHttpRequest for avoiding being visible)

cc'ing jst so that he actually sees darin's comment :)
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-17 22:27:21 PST
(In reply to comment #25)
> jst: yeah, adding a pref is a good idea, but what privacy concerns are you in
> particular thinking about?  if you are talking about URL loads going out to
> places the user didn't know about, how is that really that much different from
> JS intercepting a link click and navigating the browser elsewhere before
> redirecting the user to the place where they thought they were going?

Generally it's not, but it definately opens the door for doing that even wider (which doesn't make the problem worse, just more likely to be "abused"). But if one's got JS disabled this would let sites track what's clicked on on their pages, which w/o this gets pretty hard when browsing with JS disabled.
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-17 22:28:21 PST
Um, not sure what happened there, didn't mean to mark this fixed when adding my last comment. Reopening.
Comment 29 Hixie (not reading bugmail) 2005-12-18 21:48:53 PST
The idea is that in future we would make the status bar indicate that URIs would be pinged (so it would not be secret -- it would be more obvious to the user than it is now, in fact), and, on the long run, to provide prefs (at least hidden prefs that extensions can hook into) to make this opt-out.

However, I don't think providing the extra UI and prefs need block a v1 of this patch -- one thing at a time. ;-)

Regarding timeouts, yes, we should apply the same timeouts we normally do.
Regarding ping=" ", biesi is right that that shouldn't fetch anything.
Comment 30 Darin Fisher 2005-12-19 09:25:23 PST
> Regarding timeouts, yes, we should apply the same timeouts we normally do.

We normally do not apply any timeout to a HTTP request.  The reason is that we assume the user has the ability to cancel the request if it is taking too long.  Sometimes a server will be painfully slow, and only the user has enough information to decide whether that is okay or not.  (It turns out that there are some timeouts applied to our requests by the OS on some systems, but those timeouts are beyond our control.)
Comment 31 Hixie (not reading bugmail) 2005-12-19 12:22:21 PST
Then yeah, we should apply some arbitrary limits. Doesn't really matter what they are, so long as we've sent the packet, the response is mostly irrelevant.
Comment 32 Darin Fisher 2005-12-19 14:57:50 PST
> Then yeah, we should apply some arbitrary limits. Doesn't really matter what
> they are, so long as we've sent the packet, the response is mostly irrelevant.

The time limits would apply to DNS resolution and TCP connection establishment as well as data transmission.  That's the only way to ensure that the request does eventually get garbage collected.
Comment 33 Hixie (not reading bugmail) 2005-12-19 15:22:22 PST
That's fine. It's not supposed to be guarenteed-delivery, just best-effort.
Comment 34 Darin Fisher 2006-01-10 17:03:59 PST
Created attachment 208155 [details] [diff] [review]
v1.4 patch
Comment 35 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-10 17:46:12 PST
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

Looks good. r=jst
Comment 36 Darin Fisher 2006-01-10 17:49:28 PST
fixed-on-trunk
Comment 37 dolphinling 2006-01-11 00:37:34 PST
Forgive my ignorance, but does this apply to thunderbird as well? _Should_ it?
Comment 38 Darin Fisher 2006-01-11 07:49:11 PST
Good catch.  We should set the pref to false in thunderbird.
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2006-01-11 07:53:07 PST
What about seamonkey mailnews?  I don't see how that can disable this while letting seamonkey browser still use it; we should probably disable ping in both until it can be disabled on a per-docshell basis.  And file a bug to be able to do that -- embeddors will want it.
Comment 40 Darin Fisher 2006-01-11 08:01:32 PST
Created attachment 208201 [details] [diff] [review]
patch to disable <a ping> in tbird [fixed-on-trunk]
Comment 41 Scott MacGregor 2006-01-11 08:04:03 PST
Comment on attachment 208201 [details] [diff] [review]
patch to disable <a ping> in tbird [fixed-on-trunk]

thanks darin.
Comment 42 Darin Fisher 2006-01-11 09:07:19 PST
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

Requesting approval for FF2.
Comment 43 Darin Fisher 2006-01-11 09:09:27 PST
Marking FIXED now that the tbird "regression" is checked in.
Comment 44 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-01-11 09:23:13 PST
(In reply to comment #29)
> However, I don't think providing the extra UI and prefs need block a v1 of this
> patch -- one thing at a time. ;-)

Do you feel that the extra UI and prefs (and the testing feedback sought in comment 0) should block approval in Fx2?  I do, at first take.
Comment 45 Daniel Veditz [:dveditz] 2006-01-11 11:01:49 PST
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

we're currently giving 1.8.1 approval to fixes that are also going to be accepted on 1.8.0.x. For other general FF2 work Brendan says to wait until the dual-branch-checkin mechanism is in place.
Comment 46 Daniel Veditz [:dveditz] 2006-01-11 22:32:31 PST
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

a- is not quite what I meant.
Comment 47 Ian Neal 2006-01-12 15:18:33 PST
Created attachment 208310 [details] [diff] [review]
Disable in SeaMonkey for the moment (Checked in trunk)

Disables by setting pref to false in browser-prefs.js
Comment 48 Ian Neal 2006-01-12 16:14:10 PST
Comment on attachment 208310 [details] [diff] [review]
Disable in SeaMonkey for the moment (Checked in trunk)

Checking in (trunk)
browser-prefs.js;
new revision: 1.40; previous revision: 1.39
done
Comment 49 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-13 15:24:08 PST
(In reply to comment #39)
> What about seamonkey mailnews?  I don't see how that can disable this while
> letting seamonkey browser still use it

APP_TYPE_MAIL?
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2006-01-13 20:58:04 PST
Yeah.  I want to eliminate those constants altogether.  Docshell code shouldn't have a hardcoded list of app types, imo; it should expose capabilities and let the consumer enable or disable them...
Comment 51 bugzilla 2006-01-18 08:13:39 PST
****
Comment 52 David Aznar Reguero 2006-01-18 08:14:31 PST
What's the real reasoning behind adding this spyware?
Slashdot already take a look at this one and people didn't like it:
http://yro.slashdot.org/yro/06/01/18/1427212.shtml

If you are going to continue adding spyware instead of closing real bugs or adding real features please let people know this so they can choose another browser.
Comment 53 Darin Fisher 2006-01-18 08:21:26 PST
David, let's discuss that on my weblog (http://weblogs.mozillazine.org/darin/archives/009594.html) or on slashdot.  Not here in Mozilla's bug database.  Thanks!
Comment 54 bugzilla 2006-01-18 08:40:30 PST
Look who is whatwg: http://whatwg.org/charter#member

Look who worked on this bug!


Also, Darin Fisher wrote: This change is being considered in large part because some very popular websites have asked for a solution to this problem. (http://weblogs.mozillazine.org/darin/archives/009594.html)

Goodbye Firefox, Hello Opera.
Comment 55 Bradley Chapman (not reading bugmail, still gone but not forever) 2006-01-18 08:47:29 PST
Well, it seems that Slashdot has managed to incite a firestorm in a lighter...

What sort of UI component could be reasonably used? Could a <browsermessage/> be displayed, listing the URIs being pinged? Or would it be better to use the statusbar to display text like "Pinging <URI>"? Perhaps the favicon could blink, or the URL bar change colour, or...
Comment 56 Andrew Roazen 2006-01-18 10:41:18 PST
(In reply to comment #53)
> David, let's discuss that on my weblog
> (http://weblogs.mozillazine.org/darin/archives/009594.html) or on slashdot. 
> Not here in Mozilla's bug database.  Thanks!

No, let's air the dirty laundry here. 

It isn't HTML, it wasn't requested by users, it behaves silently without a public preference to disable it, and every public forum is filled with people who hate it. I *am* one of those developers who wants to track clicks and even I think this is despicable.
Comment 57 Boris Zbarsky [:bz] (still a bit busy) 2006-01-18 11:42:22 PST
I'd like to think the three people who commented without bothering to think about the fact that this feature is NOT DONE YET (the UI still needs to be done), without realizing that Opera is just as represented in WhatWG as anyone else, and clearly without reading the discussion on the WhatWG lists that led up to the ping thing being specified.  Had they done this last they would have realized that this is actually better behaved with regard to users (e.g. can be disabled) than the current ways of tracking with server-side redirects, etc.  Please do try to think with your heads, not other body parts.

Darin, given the tbird and seamonkey thing, I'd say we should default this to off in docshell so apps have to opt in via prefs (and remove the pref in all.js, replacing it with one in whatever the Firefox-specific config file is).  Otherwise we have issues in things like Evolution, Nvu, etc as soon as they upgrade to a 1.9 (or 1.8.1?) Gecko.
Comment 58 Jerry Baker 2006-01-18 11:43:58 PST
Is <a href="..." ping="http://www.foo.com/test/html?THISISAPING"> supposed to issue a POST request instead of a GET? That's what it does. Also, using other protocols in the ping attribute launches the associated application (like mailto:).
Comment 59 Boris Zbarsky [:bz] (still a bit busy) 2006-01-18 12:00:24 PST
> Is <a href="..." ping="http://www.foo.com/test/html?THISISAPING"> supposed to
> issue a POST request 

Yes.  See the URL in the URL field of this bug.

> Also, using other protocols in the ping attribute launches the associated
> application 

Could you please file a new bug on that, and mark it as blocking this one?  That's basically the same problem as <iframe src="mailto:...">; we should fix them both...
Comment 60 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-30 15:32:47 PST
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

I agree with bz here, let's default this off and let apps opt in if they want it. I'll + a new patch with that changed (or an additional patch or whatever).
Comment 61 Darin Fisher 2006-01-30 17:10:51 PST
jst: that be bug 324645 ;-)
Comment 62 Eric Shepherd [:sheppy] 2007-08-27 17:56:11 PDT
Is this enabled or disabled by default in Fx3?  It's coming up very soon on my schedule of getting things documented.
Comment 63 Reed Loden [:reed] (use needinfo?) 2007-08-27 18:42:05 PDT
(In reply to comment #62)
> Is this enabled or disabled by default in Fx3?  It's coming up very soon on my
> schedule of getting things documented.

Enabled by default.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/app/profile/firefox.js&rev=1.197&mark=477#477
Comment 64 Alan Peery 2007-11-25 03:24:33 PST
This must be disabled by default in Fx3, unless you want a firestorm of controversy.  If you need a reminder, look at http://weblogs.mozillazine.org/darin/archives/009594.html.

Note You need to log in before you can comment on or make changes to this bug.