Closed Bug 319368 Opened 19 years ago Closed 18 years ago

Implement <a ping>

Categories

(Core :: DOM: Core & HTML, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: ian, Assigned: darin.moz)

References

()

Details

Attachments

(3 files, 4 obsolete files)

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.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Attached patch v1 patch (obsolete) — Splinter Review
Attachment #205185 - Flags: review?(cbiesinger)
+  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.
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).
IMHO it should only fire for user and scripted activations of <a> elements and <area> elements (<area> is not yet defined in the spec).
> +  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?
Seems OK to me if Ian doesn't see any issues with it...
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).
Attached patch v1.1 patch (obsolete) — Splinter Review
In my local tree, I have added NS_DiscardSegment to xpcom/build/dlldeps.cpp as well.
Attachment #205185 - Attachment is obsolete: true
Attachment #205323 - Flags: review?(cbiesinger)
Attachment #205185 - Flags: review?(cbiesinger)
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).
> 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!
Yeah, working without IsContentOfType is hard.... ;)
Attached patch v1.2 patch (obsolete) — Splinter Review
Boris: you willing to SR this?
Attachment #205323 - Attachment is obsolete: true
Attachment #205335 - Flags: review?(cbiesinger)
Attachment #205323 - Flags: review?(cbiesinger)
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
Attachment #205335 - Flags: superreview+
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
> 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.
> 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...
> 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?
Attached patch v1.3 patch (obsolete) — Splinter Review
With Tag() and more reuse of NS_DiscardSegment.
Attachment #205335 - Attachment is obsolete: true
Attachment #205421 - Flags: review?(cbiesinger)
Attachment #205335 - Flags: review?(cbiesinger)
I also switched that nsString to a nsAutoString.
I think it's better to go with nsAutoString, yeah....
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! ;-)
Attachment #205421 - Flags: review?(cbiesinger) → review?(jst)
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.
Attachment #205421 - Flags: review?(jst) → review-
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
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?
> 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?
(that JS could also use XMLHttpRequest for avoiding being visible)

cc'ing jst so that he actually sees darin's comment :)
(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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Um, not sure what happened there, didn't mean to mark this fixed when adding my last comment. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
> 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.)
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.
> 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.
That's fine. It's not supposed to be guarenteed-delivery, just best-effort.
Attached patch v1.4 patchSplinter Review
Attachment #205421 - Attachment is obsolete: true
Attachment #208155 - Flags: review?(jst)
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

Looks good. r=jst
Attachment #208155 - Flags: review?(jst) → review+
fixed-on-trunk
Status: REOPENED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Forgive my ignorance, but does this apply to thunderbird as well? _Should_ it?
Good catch.  We should set the pref to false in thunderbird.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #208201 - Flags: review?(mscott)
Comment on attachment 208201 [details] [diff] [review]
patch to disable <a ping> in tbird [fixed-on-trunk]

thanks darin.
Attachment #208201 - Flags: review?(mscott) → review+
Attachment #208201 - Attachment description: patch to disable <a ping> in tbird → patch to disable <a ping> in tbird [fixed-on-trunk]
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

Requesting approval for FF2.
Attachment #208155 - Flags: approval1.8.1?
Marking FIXED now that the tbird "regression" is checked in.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
(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 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.
Attachment #208155 - Flags: approval1.8.1? → approval1.8.1-
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

a- is not quite what I meant.
Attachment #208155 - Flags: approval1.8.1- → approval1.8.1?
Disables by setting pref to false in browser-prefs.js
Attachment #208310 - Flags: superreview?(bzbarsky)
Attachment #208310 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 323238
Attachment #208310 - Flags: superreview?(bzbarsky) → superreview+
Attachment #208310 - Flags: review?(neil.parkwaycc.co.uk) → review+
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
Attachment #208310 - Attachment description: Disable in SeaMonkey for the moment → Disable in SeaMonkey for the moment (Checked in trunk)
(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?
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...
****
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.
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!
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.
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...
(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.
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.
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:).
> 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...
Depends on: 323924
Depends on: 323959
Depends on: 323793
Attachment #208155 - Flags: approval1.8.1? → branch-1.8.1?(jst)
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).
Attachment #208155 - Flags: branch-1.8.1?(jst) → branch-1.8.1-
jst: that be bug 324645 ;-)
Depends on: 324645
Is this enabled or disabled by default in Fx3?  It's coming up very soon on my schedule of getting things documented.
(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
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.
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.