The default bug view has changed. See this FAQ.

Implement <a ping>

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM: Core & HTML
--
enhancement
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 1

12 years ago
Created attachment 205185 [details] [diff] [review]
v1 patch
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).
(Reporter)

Comment 4

12 years ago
IMHO it should only fire for user and scripted activations of <a> elements and <area> elements (<area> is not yet defined in the spec).
(Assignee)

Comment 5

12 years ago
> +  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...
(Reporter)

Comment 7

12 years ago
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).
(Assignee)

Comment 8

12 years ago
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.
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).
(Assignee)

Comment 10

12 years ago
> 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.... ;)
(Assignee)

Comment 12

12 years ago
Created attachment 205335 [details] [diff] [review]
v1.2 patch

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
(Assignee)

Comment 15

12 years ago
> 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...
(Assignee)

Comment 17

12 years ago
> 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?
(Assignee)

Comment 18

12 years ago
Created attachment 205421 [details] [diff] [review]
v1.3 patch

With Tag() and more reuse of NS_DiscardSegment.
Attachment #205335 - Attachment is obsolete: true
Attachment #205421 - Flags: review?(cbiesinger)
Attachment #205335 - Flags: review?(cbiesinger)
(Assignee)

Comment 19

12 years ago
I also switched that nsString to a nsAutoString.
I think it's better to go with nsAutoString, yeah....
(Assignee)

Comment 21

11 years ago
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?
(Assignee)

Comment 25

11 years ago
> 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
Last Resolved: 11 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 → ---
(Reporter)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
> 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.)
(Reporter)

Comment 31

11 years ago
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.
(Assignee)

Comment 32

11 years ago
> 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.
(Reporter)

Comment 33

11 years ago
That's fine. It's not supposed to be guarenteed-delivery, just best-effort.
(Assignee)

Comment 34

11 years ago
Created attachment 208155 [details] [diff] [review]
v1.4 patch
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+
(Assignee)

Comment 36

11 years ago
fixed-on-trunk
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 37

11 years ago
Forgive my ignorance, but does this apply to thunderbird as well? _Should_ it?
(Assignee)

Comment 38

11 years ago
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.
(Assignee)

Comment 40

11 years ago
Created attachment 208201 [details] [diff] [review]
patch to disable <a ping> in tbird [fixed-on-trunk]
Attachment #208201 - Flags: review?(mscott)

Comment 41

11 years ago
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+
(Assignee)

Updated

11 years ago
Attachment #208201 - Attachment description: patch to disable <a ping> in tbird → patch to disable <a ping> in tbird [fixed-on-trunk]
(Assignee)

Comment 42

11 years ago
Comment on attachment 208155 [details] [diff] [review]
v1.4 patch

Requesting approval for FF2.
Attachment #208155 - Flags: approval1.8.1?
(Assignee)

Comment 43

11 years ago
Marking FIXED now that the tbird "regression" is checked in.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 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?

Comment 47

11 years ago
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
Attachment #208310 - Flags: superreview?(bzbarsky)
Attachment #208310 - Flags: review?(neil.parkwaycc.co.uk)

Updated

11 years ago
Blocks: 323238
Attachment #208310 - Flags: superreview?(bzbarsky) → superreview+

Updated

11 years ago
Attachment #208310 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 48

11 years ago
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...

Comment 51

11 years ago
****

Comment 52

11 years ago
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.
(Assignee)

Comment 53

11 years ago
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

11 years ago
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...

Comment 56

11 years ago
(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.

Comment 58

11 years ago
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...
(Assignee)

Updated

11 years ago
Depends on: 323924

Updated

11 years ago
Depends on: 323959
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 61

11 years ago
jst: that be bug 324645 ;-)
(Assignee)

Updated

11 years ago
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

Comment 64

10 years ago
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.

Updated

9 years ago
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.