100944 - search property of link object not settable from javascript

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Greg Allen]

The following Javascript error occurs when trying to set the search property of
a HTML link:

Error: setting a property that has only a getter
Source File: http://intranet.csfb.net/gssg/css/dev/summit/support/EoDButtons.htm
Line: 33

where line 33 is lnk.search = lnk. blah below:

function updateLink(lnk, sel) {
	var ind = lnk.search.indexOf('&env')
	if (ind != -1)
		lnk.search = lnk.search.substring(0,ind) +
'&env='+sel.options[sel.selectedIndex].value
	else
		lnk.search += '&env='+sel.options[sel.selectedIndex].value
}

function updateLinks(sel) {
	var lnk, ind
	for (var i = 0; i < document.links.length; i++) {
		lnk = document.links[i]
		if (lnk.pathname.indexOf('endofday') != -1)
			updateLink(lnk, sel)
	}
}


This works in IE4, IE5 and Netscape 4. Javascript documentation also suggests
this (setting link.search) is permissable.

Comment 1

[Greg Allen]

*** Bug 100941 has been marked as a duplicate of this bug. ***

Comment 2

[Greg Allen]

*** Bug 100942 has been marked as a duplicate of this bug. ***

Comment 3

[Fabian Guisset]

search is implemented as a readonly attribute of links in Mozilla. Could you
please point us to the documentation where it says it should set'able?
Thanks in advance!

Comment 4

[Greg Allen]

I have a local copy of:
Client-Side JavaScript Reference v1.3
which must have been downloaded off the Netscape site a while back.
It says:

search
A string beginning with a question mark that specifies any query information in
the URL.

Property of

Link

Implemented in

JavaScript 1.0

Security
JavaScript 1.1. This property is tainted by default. For information on data
tainting, see the Client-Side JavaScript Guide.

Description
The search property specifies a portion of the URL. This property applies to
http URLs only.

The search property contains variable and value pairs; each pair is separated by
an ampersand. For example, two pairs in a search string could look like the
following:

?x=7&y=5

You can set the search property at any time, although it is safer to set the
href property to change a location. If the search that you specify cannot be
found in the current location, you get an error.

See Section 3.3 of RFC 1738
(http://www.cis.ohio-state.edu/htbin/rfc/rfc1738.html) for complete information
about the search.

Greg.

Comment 5

[Boris Zbarsky [:bzbarsky]]

confirmin for now while you guys hash it out.  :)

Comment 6

[Johnny Stenback (:jst)]

Hmm, I wonder why this property was made readonly in mozilla, I'm fine with
making the link href related properties that were settable in 4x read-write. If
we do this we should look for all interfaces that have 'search' n' friends
attributes, I know at least nsIDOMLocation has them, and it looks like they're
writable there. Odd.

Fabian, wanna fix this one? Look in nsLocation.cpp for examples :-)

Comment 7

[Fabian Guisset]

I'm your man.
->DOM Level 0 P2

Comment 8

[Fabian Guisset]

Attachment: Make search set'able for area and anchor elements. Any other?

Comment 9

[Fabian Guisset]

Comment on attachment 50392 [details] [diff] [review]
Make search set'able for area and anchor elements. Any other?

Forget about this one.

Comment 10

[Fabian Guisset]

Attachment: Should be better. Not sure about the string usage. It compiles and works, at least ;-)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 50415 [details] [diff] [review]
Should be better. Not sure about the string usage. It compiles and works, at least ;-)

>+  nsresult rv = NS_OK;
>+
>+  rv = GetHref(href);

how about 

nsresult rv = GetHref(href); 

>+  if (NS_SUCCEEDED(rv)) {
>+    nsCOMPtr<nsIURI> uri;
>+
>+    rv = NS_NewURI(getter_AddRefs(uri), href);
>+
>+    if (uri) {
>+      nsXPIDLCString search;
>+      nsCOMPtr<nsIURL> url(do_QueryInterface(uri, &rv));
>+
>+      if (url) {
>+        rv = url->SetQuery(NS_ConvertUCS2toUTF8(aSearch).get());
>+
>+        href.Append(aSearch);
>+        SetHref(href);

Um.  did you forget to get the search back out of the nsIURL??

Instead of using Append, you can just do SetHref(href + aSearch)
That's faster.

>+  nsresult rv = NS_OK;
>+
>+  rv = GetHref(href);

again, you can probably combine those two....

>+        rv = url->SetQuery(NS_ConvertUCS2toUTF8(aSearch).get());
>+
>+        href.Append(aSearch);
>+        SetHref(href);

Again, don't you need to get the new href out of the url?

Or am I completely missing the point of these urls you're creating?

Comment 12

[Johnny Stenback (:jst)]

nsIDOMLocation has hash, host, hostname, href, pathname, port, protocol and
search as readwrite properties, not sure what 4x does, and I don't have time to
look that up now. Does anyone know?

Comment 13

[Fabian Guisset]

Yo Boris :-)
I guess it depends what this property does.
anchor.search = "?hello"; should change the href attribute of the anchor element to
the current href + "?hello". So all I'm doing is nsIURL->SetQuery (just copied
from nsLocation for this one), then appending aSearch (which is the string that
contains the search ?hello) to the original href. Then I'm simply setting the
href attribute to that new string. Did I misunderstand the doc that Greg pasted?

Comment 14

[Fabian Guisset]

Comment on attachment 50415 [details] [diff] [review]
Should be better. Not sure about the string usage. It compiles and works, at least ;-)

You were right. New patch coming up.

Comment 15

[Fabian Guisset]

Attachment: Please look carefully at the string usage. Arghh.

Comment 16

[Fabian Guisset]

This one works correctly. However I'm not sure about the string usage, specially
the use of nsCRT::free (I just copied the majority of the other GetSpec users)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 50462 [details] [diff] [review]
Please look carefully at the string usage. Arghh.

>+      nsXPIDLCString search;

You aren't using this, are you?

>+        char *newHref;
>+        uri->GetSpec(&newHref);
>+        SetHref(NS_ConvertUTF8toUCS2(newHref));
>+        nsCRT::free(newHref);

This is correct, techincally, but I would do something like:

nsXPIDLCString newHref;
uri->GetSpec(getter_Copies(newHref));
SetHref(NS_ConvertUTF8toUCS2(newHref));

Then there is no need to free |newHref|; it will free itself.

>+        char *newHref;
>+        uri->GetSpec(&newHref);
>+        SetHref(NS_ConvertUTF8toUCS2(newHref));
>+        nsCRT::free(newHref);

same here.  use an nsXPIDLCString

Fix those and then I'll try to break it?  :)

Comment 18

[Fabian Guisset]

Attachment: Try to break this one ;-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Bad broken Testcase

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Um. Real testcase. Ignore that first one...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 50711 [details] [diff] [review]
Try to break this one ;-)

r=bzbarsky

Comment 22

[Fabian Guisset]

No sr= yet => mozilla0.9.6

Comment 23

[Johnny Stenback (:jst)]

Fix checked in, leaving bug open to do the rest of the URL related properties,
see nsIDOMLocation for sample, and verify that the properties are settable in IE
or NS4x before making the change in mozilla.

Comment 24

[Fabian Guisset]

Attachment: Proposed fix

Comment 25

[Fabian Guisset]

Christian Biesinger ([e-mail]) kindly volunteered to do the coding.
Here's the patch he sent me. It compiles (I applied it), but I still need to
test it. At my request it includes a few raw pointer -> nsCOMPtr changes.
Thanks biesi!
I'll review it asap.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55451 [details] [diff] [review]
Proposed fix

>+      char* scheme = nsCRT::strdup(NS_ConvertUCS2toUTF8(aProtocol).get());
>+      if (!scheme)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      char* c;
>+      if ((c = strchr(scheme, ':')))
>+        *c = '\0';
>+
>+      uri->SetScheme(scheme);
>+
>+      nsCRT::free(scheme);

ugh.  :)  First of all, how does NS4 handle these?  Would we possibly
just want to throw an error if the aProtocol string includes a ":"?
If we _do_ want to do what you do here, how about something like:

nsAString::const_iterator start, end;
aProtocol.BeginReading(start);
aProtocol.EndReading(end);
nsAString::const_iterator iter(start);
FindCharInReadable(':', iter, end);
uri->SetScheme(NS_ConvertUCS2toUTF8(Substring(start, iter))).get());

>+  // Can't simply call nsURI::SetHost, because that would treat the name as an
>+  // IPv6 address (like http://[server:443]/)

well... won't your code break somebody using SetHost with an IPv6 
address?

You may want to create an nsIStandardURL or nsIURIFixup and use those
to parse the host, then just ask them for the host and port.

Again, how does ns4 handle this?

Same comments apply to <area> elements.

Comment 27

[Boris Zbarsky [:bzbarsky]]

biesi, could you please attach a testcase here if you have one?  

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Testcase for all <a> and <area> URL-related properties

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Patch which addresses bz's comments

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55717 [details] [diff] [review]
Patch which addresses bz's comments

>+      uri->SetScheme(NS_ConvertUCS2toUTF8(Substring(start, iter)).get());

bbaetz was saying that this may be unsafe.  Please check with him as to
whether it's OK in this case.

>+      nsAutoString url;
>+      char* ptr;
>+      uri->GetScheme(&ptr);
>+      url.Assign(NS_ConvertASCIItoUCS2(ptr));
>+      nsCRT::free(ptr);
>+
>+      url += NS_LITERAL_STRING("://");
>+
>+      uri->GetPreHost(&ptr);
>+      url += NS_ConvertUTF8toUCS2(ptr);
>+      nsCRT::free(ptr);
>+
>+      url += aHost;
>+
>+      uri->GetPath(&ptr);
>+      url += NS_ConvertUTF8toUCS2(ptr);
>+      nsCRT::free(ptr);
>+ 
>+      rv = SetHref(url);

Hmm... This has two problems:
1) it's OK as long as your URL is 64 chars or fewer.  Once it's
   bigger than that this is going to be slow.  Try something more like:

nsXPIDLCString scheme;
uri->GetScheme(getter_Copies(scheme));
nsXPIDLCString preHost;
uri->GetPreHost(getter_Copies(preHost));
nsXPIDLCString path;
uri->GetPath(getter_Copies(path));

rv = SetHref(NS_ConvertUCS2toUTF8(scheme) + ... + aHost + ...)

2)  You're forcing "://" on this url.  That may well not be compatible
    with the scheme that's being set (eg mailto:)

>+      const char* buf = NS_ConvertUCS2toUTF8(aPort).get();

Bad.  The NS_ConvertUCS2toUTF8 temporary goes away after this line.
So you have a pointer into memory you don't really own.  It may work.
It may not.  I'd suggest something like:

nsAutoString port(aPort);
if ((! port.IsEmpty()) && port.First() == ':') {
  port.Cut(0,1);
}
uri->SetPort(port.ToInteger());

Failing that, at least keep the results of the NS_ConvertUCS2toUTF8
in an nsDependentString while you're working with the pointer

And all the same for Area elements, of course.  :)

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Patch v3

Comment 32

[Johnny Stenback (:jst)]

Given the amount of new code here we should split part of these methods into
nsGenericHTMLElement so that nsHTMLAnchorElement and nsHTMLAreaElement could use
the same code to whatever extent it's possible.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

jst: This would indeed be a good idea; however, I think it's rather difficult to do.

I suggest that you open a new bug for it.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55821 [details] [diff] [review]
Patch v3

>+      PRInt32 port, rv;
>+      port = nsString(aPort).ToInteger(&rv);
>+      if (NS_FAILED(rv))
>+        return rv;

I know that ToInteger wants a PRInt32*, but please just use the rv you
already have.  The types are compatible, and if you're gonna use it as
an nsresult later it's better to have it be an nsresult.
And return NS_OK, here (per discussion on IRC -- NS4 silently does the
wrong thing given bad input here).

Fix that in both files, and r=bzbarsky

Now you just have to make jst happy.  :)

Comment 35

[Johnny Stenback (:jst)]

Splitting the bulk of the code in these methods into separate reusable helper
methods is everything but difficult to do. As an exacmple:

// static
nsresult
nsGenericHTMLElement::SetProtocolInHrefString(const nsAReadableString &aHref,
                                              nsAWritableString &aResult)
{
  nsCOMPtr<nsIURI> uri;
  rv = NS_NewURI(getter_AddRefs(uri), href);
  if (NS_FAILED(rv)) {
    return rv;
  }

  nsAString::const_iterator start, end;
  aProtocol.BeginReading(start);
  aProtocol.EndReading(end);
  nsAString::const_iterator iter(start);
  FindCharInReadable(':', iter, end);
  uri->SetScheme(NS_ConvertUCS2toUTF8(Substring(start, iter)).get());
   
  nsXPIDLCString newHref;
  uri->GetSpec(getter_Copies(newHref));

  aResult.Assign(NS_ConvertUTF8toUCS2(newHref));

  return NS_OK;
}

and then you make the new SetProtocol() methods look something like this:

NS_IMETHODIMP
nsHTMLAnchorElement::SetProtocol(const nsAReadableString& aProtocol)
{
  nsAutoString href, new_href;
  nsresult rv = GetHref(href);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = SetProtocolInHrefString(href, new_href);

  if (NS_FAILED(rv))...

  return SetHref(new_href);
}

And something similar for the rest of the methods.

Do that and I'll sr.

Comment 36

[Johnny Stenback (:jst)]

And add a aResult.Truncate() to the beginning of
nsGenericHTMLElement::SetProtocolInHrefString().

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Patch v4

Comment 38

[Fabian Guisset]

Comment on attachment 57304 [details] [diff] [review]
Patch v4

quick nit before I review the string hell (caramba): all
if (NS_FAILED(rv)) return foo;
should be
NS_ENSURE_SUCCESS(rv, foo); (no need to attach a  new patch)

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

I didn't use NS_ENSURE_SUCCESS because that asserts if the function failed. I
think that an assertion isn't necessary.

Opinions?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 57304 [details] [diff] [review]
Patch v4

>+nsGenericHTMLElement::SetPathnameInHrefString(const nsAReadableString &aHref,
>+                                 const nsAReadableString &aPathname,
>+                                 nsAWritableString &aResult)

Fix the indentation

>+  rv = SetProtocolInHrefString(href, aProtocol, new_href);
>+  if (NS_FAILED(rv))
>+    return NS_OK;

please add a comment (here and elsewhere where you do this)
explaining that you're not throwing an exception here to keep 
compat with 4.x....

Do those and r=bzbarsky.  No need for a new patch.

Comment 41

[Johnny Stenback (:jst)]

What bz said, and also, please change code like:

+  char *search;
+  rv = url->GetEscapedQuery(&search);
...
+  nsCRT::free(search);

to:

+  nsXPIDLCString search;
+  rv = url->GetEscapedQuery(getter_Copies(search));
...

sr=jst

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Final Patch.

Patch which addresses bz's and jst's comments.
Has r=bz sr=jst; could anybody check this in?

Comment 43

[Boris Zbarsky [:bzbarsky]]

Taking this to check it in per biesi's request

Comment 44

[Boris Zbarsky [:bzbarsky]]

Checked in

Comment 45

[Sivakiran Tummala]

reopening bug failed on win2k build 2001-11-26.

Comment 46

[Sivakiran Tummala]

reopening bug failed on win2k build 2001-11-26.

Comment 47

[Boris Zbarsky [:bzbarsky]]

which particular testcase failed?  What was the error?

Comment 48

[Sivakiran Tummala]

attachment id 50721 failed. I am also creating a new attachment testcase

Comment 49

[Sivakiran Tummala]

Attachment: testcase

Comment 50

[Boris Zbarsky [:bzbarsky]]

Ugh.  This works for me in a Nov. 26 morning build, and breaks in a Nov. 27
morning build.  The problem is that we are doing SetQuery("id=100") on a uri and
the uri parser is escaping the '=' to "%3D".

Darin?

Comment 51

[Darin Fisher]

that bug should be fixed now... was a typo in some of changes made to
nsStandardURL.cpp ... please try again with a more recent build.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Indeed, a Nov. 29 build works once again.  sivarikan, could you re-verify?

Comment 53

[Sivakiran Tummala]

verified on build 2001-11-29-09 trunk.