276103 - Using substrings inside link() doesn't work

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Gianugo Rabellino]

Weird one. If you try to build a link using var.link(destination) where
"destination" comes from a substring operation, the originating (before the
substring) string is used as the link target. This happens *even* if you assign
the substring value to a different var. So:

      function makeLink() {
        var testString = "test|string";
        var idx = testString.indexOf("|");
        var link = testString.substring(0, idx);
        var desc = testString.substring(idx + 1);

        document.write(desc.link(link));
      }

produces 

<a href="test|string">string</a>

instead than the expected (and working on Safari/IE, both Mac and Win)

<a href="test">string</a>

Will attach an HTML test case

Comment 1

[Gianugo Rabellino]

Attachment: HTML Test case showing the problem

Comment 2

[Gianugo Rabellino]

getting weirder. This works:

      function makeLink() {
        var testString = "test|string";
        var idx = testString.indexOf("|");
        var link = testString.substring(0, idx);
        var desc = testString.substring(idx + 1);

        alert(link);

        document.write(desc.link(link));
      }

So:

1. if you omit the alert, desc.link(link) = <a href="test|string">string</a>
2. if you use the alert, desc.link(link) = <a href="test">string</a>

How can an alert() fix the problem?

Comment 3

[Brendan Eich [:brendan]]

See bug 276030 for a coincidence: on the cvs.mozilla.org trunk I have disabled
at compile time these old HTML string helper methods (link, anchor, etc.).  Not
due to bugs, but in the hope that the lack of a de-jure standard meant that
other browsers didn't implement them.  Fat chance!

When it rains, it pours -- I wonder why folks are playing with these old HTML
helpers now (this bug was introduced on 2001/10/25).  Or perhaps this is code
being run in a Mozilla product for the first time.

Anyway, this is a jsstr.c bug where old code that assumed all JSStrings were
NUL-terminated ran into the dependent string change done years later to optimize
concatenation (see bug 56940).  A dependent string shares a reference into
another string's buffer, so there is no NUL backstop -- but JS allows NULs in
strings, so the tagify static function in jsstr.c was always on thin ice using
js_strlen(param).

Gianugo, did you see this in Firefox 1.0?

At least there's a workaround: without the alert, you should be able to make the
dependent string convert to an immutable (independent, NUL-terminated) string by
using it to index into a property: var junk = {}; junk[link] = 42 (or similar).

Sorry about this bug, it's nine and a half years old.

/be

Comment 4

[Gianugo Rabellino]

Brendan, yes, this is Firefox 1.0 (Mozilla/5.0 (Macintosh; U; PPC Mac OS X
Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0). Tried on a number of other
Firefox/Mozilla combinations though...

Interesting workaround though, but I've already converted everything to good old
link building via string concatenation. Thanks anyway for your support, good to
know I have another way out (I'm working on other's code).

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

If you have a lot of code that uses the HTML string helpers, attachment 169599 [details]
from bug 276030 should save having to expand the calls into string
concatenations all over the place.

This fix is good for 1.7.6 and any other active branches.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: alternate fix, larger but safe

Non-minimal fix, more efficient, suggested by dbaron.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 169641 [details] [diff] [review]
fix

Requested approval too early -- sorry, stay tuned to this bug.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 169641 [details] [diff] [review]
fix

Never mind, the patch dbaron suggested is better all around.  This is smaller
in source chars changed, but so what?  It hides an OOM error, and flattens a
dependent string needlessly, compared to the 2nd patch.

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 169643 [details] [diff] [review]
alternate fix, larger but safe

r=shaver across the chronal barrier!  (with a warning fix: the JSString *param
argument to tagify can't be a pointer to const struct, because JSSTRING_CHARS
calls js_GetDependentStringChars, which takes a mutable string).

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed, closing but keeping bug on branch approval radar.

/be

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 169643 [details] [diff] [review]
alternate fix, larger but safe

a=caillon for 1.7.6.  Please land this weekend.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-276103.js

Gianugo, with your permission this will go in the javascript test library.

Comment 13

[Gianugo Rabellino]

No problem at all, consider it officially contributed.

Comment 14

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-276103.js checked in.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 169643 [details] [diff] [review]
alternate fix, larger but safe

This hasn't landed yet, so changing to minus.

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.