LiveConnect top-level property names predefined readonly

VERIFIED FIXED

Status

Core Graveyard
Java: Live Connect
P3
normal
VERIFIED FIXED
18 years ago
8 years ago

People

(Reporter: brendan, Assigned: rogerl (gone))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-] r=mccabe,a=brendan, URL)

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
This breaks backward compatibility.  See the article at the bug URL.  See also 
bug 35707, which reports a lower-priority symptom (performance degraded due to 
eager initialization) -- the fix for both may be the same one sketched in 35707.

/be
(Reporter)

Comment 1

18 years ago
A quicker fix for just this bug, and not for 35707, is to make java, sun, and 
netscape read-write properties.  Adding rtm keyword, cc'ing ekrock.

/be
Keywords: rtm
(Assignee)

Comment 2

18 years ago
Created attachment 16336 [details] [diff] [review]
Supply JSPROP_READONLY as a parameter to define_JavaPackage
(Reporter)

Comment 3

18 years ago
Looks good, except for this style nit:

+            parent_obj = define_JavaPackage(cx, parent_obj, simple_name, path, 
flags,
+                                                                    
package_def->access);

The argument on the second line doesn't underhang so as to align with the first 
argument -- instead it's way indented to the right.  Or maybe evil tabs are 
being used at a small modulus?

a=brendan@mozilla.org, get an r= from jst or from anyone liveconnect-aware, and 
then an [rtm+] when this is in the trunk.  Cc'ing beard.

/be

Comment 4

18 years ago
r=mccabe, looks like the right minimal-yet-general fix.
(Assignee)

Comment 5

18 years ago
Checked in to trunk, waiting for rtm+...

Comment 6

18 years ago
Marking rtm+, to satisy backward compatibility.
Whiteboard: [rtm+]

Comment 7

18 years ago
It's not obvious to me why end-users care a lot about this. Is there some
commonly visited page which is crippled by this bug? Marking [rtm need info]
(Reporter)

Comment 8

18 years ago
Cc'ing attinasi@netscape.com, he can confirm whether the test page in his post
(see the bug URL) was based on a real-world page.

Maybe ekrock has some data.

I don't see why this fix carries enough risk to be worth putting off, even if
"end-users" won't notice the bug (yet, so far as we can tell).  My 2 cents.

/be
Keywords: 4xp
1) We know that this problem creates a backward incompatibility.
2) We *don't* know off the top of our heads how many pages will be affected. 
Could be many, could be few. Unfortunately I have no cycles to do this research 
at this stage of the game.
3) There's one way to find out for sure how widespread this backward 
incompatibility will be--ship it! ;-< I'd rather not Go There.
4) Given that the patch is well-reviewed and judged low-risk, could we please 
accept for RTM?
John, George, Ed: Can you provide input about the impact of not accepting this 
patch for RTM?

Comment 11

18 years ago
Given that the PDT has decided that this is the time to go into "total bastard
mode" (a.k.a. Long period of testing in the trunk? Crashes everywhere without
the patch? Nah. Still won't accept it.), we should just rtm- this bug now and
get it over with.

Let the remaining bugs on the rtm radar be bugs that we will fight for with
every inch of our lives.
This bug needs r= and sr= if it is to have any hope of being accepted for the
branch. Can anyone please provide the needed r= and sr=?

Comment 13

18 years ago
Bastards marking [rtm-] since no reviews are available, and no frequently
visited sites are known to be damaged by this bug.
Whiteboard: [rtm need info] → [rtm-]
(Reporter)

Comment 14

18 years ago
Phil, ekrock: I gave a= (sr=, same thing) and mccabe gave r=.  Did you guys not 
read up to where the patch was attached?  The fix has been on the trunk since 
10/9.  I don't understand the hold-up after that.

/be
Whoops, my oversight. Thanks for the catch, Brendan. Marking r=mccabe,a=brendan
 and resetting to rtm+ to trigger re-evaluation since phil and I overlooked the
review comments. So we *do* have reviews.

Documentation of real-world impact: from the article cited above: appears that
this bug causes navigator.appName to return the wrong value in Seamonkey, which
is quite scary as this value is frequently used for client sniffing and
determining which of multiple code forks is followed in client-side JavaScript
code. Pasting above article inline:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3c.org/TR/html4/strict.dtd">
<HTML>
<HEAD>
<TITLE>JavaScript Bug</TITLE>
<script language="JavaScript">
browser = navigator.appName;
netscape = "Netscape";
if (browser == netscape) {
dump( "\n\n*** Netscape ***\n\n");
}
else {
dump( "\n\n*** value of variable netscape is "+netscape+" ***\n\n");
}
</script>
</HEAD>
<BODY>
<p>Check the console: should show a dump of *** Netscape *** and not</p>
<p>a dump of *** value of variable... ***</p>
</BODY>
</HTML>
Whiteboard: [rtm-] → [rtm+] r=mccabe,a=brendan
(Assignee)

Comment 16

18 years ago
ekrock wanted me to annotate with a workaround -

A web-site using 'netscape', 'sun' or 'java' as top-level property names in 
any Javascript code must re-write using different names instead. So 

browser = navigator.appName;
netscape = "Netscape";
if (browser == netscape)
...etc

becomes

browser = navigator.appName;
netscapeName = "Netscape";
if (browser == netscapeName) {
...etc

Comment 17

18 years ago
ekrock sez, this is specific to the case where "netscape", "java" or "packages"
is used as a variable name. Since this is an edge case, we're marking [rtm-]
Whiteboard: [rtm+] r=mccabe,a=brendan → [rtm-] r=mccabe,a=brendan
(Assignee)

Comment 18

16 years ago
This can be closed now right? The patch has been in the trunk for yonks and the 
rtm+/-'ness is so last year.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 19

16 years ago
Marking Verified Fixed.

Mozilla allows read-write properties to |java|, |sun|, and |netscape|
as suggested in Comment #1 above.  Just try this script and you get
no errors in the JS Console:

<SCRIPT>
netscape = 999;
java = 888;
sun = 777;

document.write( "<br><br>The variable |netscape| equals " + netscape);
document.write( "<br><br>The variable |java| equals " + java);
document.write( "<br><br>The variable |sun| equals " + sun);
</SCRIPT>


Furthermore, |navigator.appName| correctly gives "Netscape",
as requested by the reporter in the original newsgroup item.

NOTE: for those curious why it's not "Mozilla", see bug 61071.
Status: RESOLVED → VERIFIED

Updated

8 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.