37258 - window.toolbar must be backwardly compatible with 4.x and IE

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Description

[ppandit]

Using a debug build on Windows NT from 4/24/00.

1.) Go to www.visto.com
2.) Create an account if necessary
3.) Also create a "group"
4.) After logging in you should see different frame sets. But in mozilla, all I 
see is the frame containing the messages. The group tabs across the top and the 
menu options along the side do not exist. Those frames are empty.
5) I will add the html page but you will actually need a visto account in order 
to see the problem.

Par

Comment 1

[ppandit]

Attachment: Main page after log into visto.com

Comment 2

[troy]

Changing component

Comment 3

[Eric Pollmann]

Through some clever use of Javascript, the contents of the other frames are 
dynamically generated.  The problem appears to be that the name of the first 
frame to be generated is "toolbar", and when the script requests window.toolbar 
we give back this:

[object BarProp]

Then window.toolbar.document comes back as undefined and we later die:

JavaScript error:
file:///C:/tmp/visto_main.html line 17:

JavaScript error:
 line 0: uncaught exception: [Exception... "Access to property denied"  code: "1
010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)"  location: "file:/
//C:/tmp/visto_main.html Line: 17"]

I will reduce the test case, though it sounds a lot like bug 33650, so I'm 
passing off to Hyatt for a quick inspection.  Thanks!

Comment 4

[Eric Pollmann]

Well, this may in fact be three separate bugs:

1) window.toolbar isn't backwards compatabable with 4.x and IE
2) Something else is causing the javascript on this page not to work (???)
3) Sometime today or yesterday a change was checked in that causes a corrupt 
heap and crash when submitting this form to "javascript:doSubmit();"

_free_dbg_lk(void * 0x0361f300, int 1) line 1027 + 26 bytes
_free_dbg(void * 0x0361f300, int 1) line 970 + 13 bytes
free(void * 0x0361f300) line 926 + 11 bytes
PL_strfree(char * 0x0361f300) line 44 + 10 bytes
nsCRT::free(char * 0x0361f300) line 167 + 9 bytes
nsJSThunk::Close(nsJSThunk * const 0x035fcca0, unsigned int 2152398850) line 275 
+ 13 bytes
nsStreamIOChannel::OnStopRequest(nsStreamIOChannel * const 0x035fa654, 
nsIChannel * 0x03610070, nsISupports * 0x00000000, unsigned int 2152398850, 
const unsigned short * 0x00000000) line 635
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x0361fec0) line 
307
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x0361f320) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x0361f320) line 575 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01501160) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x130e067c, unsigned int 49233, unsigned int 0, 
long 22024544) line 1030 + 9 bytes
USER32! 77e71268()
01501160()

Comment 5

[Eric Pollmann]

Attachment: Inner frame, reduced

Comment 6

[Eric Pollmann]

Attachment: Reduced test case

Comment 7

[Eric Pollmann]

The reason for bug 2) appears to be that we don't create a document for a frame 
if the src is a javascript url.  Changing the src of the top frame in the 
reduced test case to about:blank fixes that part of the bug.  I'll open up a new 
bug for that and work on it.  Takers for 1) and 3)?

Comment 8

[David Hyatt]

vidur has magic idl flag for this.

Comment 9

[vidur (gone)]

It be called "replaceable". See 
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/Window.idl.

Comment 10

[ekrock's old account (dead)]

Vidur says that the above comment explains how to fix problem #1 of the three 
issues Eric Pollmann identified. We will use this bug to track issue #1. 
Changing summary from "Frame name 'toolbar' disallowed: visto.com broken" to 
read "[FEATURE] window.toolbar must be backwardly compatible with 4.x and IE".

Comment 11

[Peter Trudelle]

Is this really a feature, or just a defect?  In any case, to get it done someone
must nominate it for tagging by PDT, or it will get a pocket veto.

Comment 12

[ppandit]

Alot of people use visto.com. I would suggest fixing it.

Par

Comment 13

[Brendan Eich [:brendan]]

Removing bogus [FEATURE] label.

/be

Comment 14

[ekrock's old account (dead)]

Need to be b.c. for FCS. Nominating nsbeta2. Recommend 
[nsbeta2+][6/15][nsbeta3+].

Comment 15

[Peter Trudelle]

[nsbeta2+] [6/22]

Comment 16

[Jim Roskind]

Cleaning up status whiteboard and marking beta2 minus (6/22 has passed).

It sounds like this bug will burn us on other sites, as we're having a naming 
problem in JS (If I get the gist of the bug).  I'm going to nominate for beta3.

Comment 17

[Peter Trudelle]

need info: How does Nav 4 avoid a conflict with this site?

Comment 18

[Peter Trudelle]

another question: Do we need to make all of the window properties replaceable to
fix this?
cc jrgm

Comment 19

[John Morrison]

In navigator 4.x, if you have a frameset with frames having names like 
'toolbar', 'statusbar', etc. then the property window.toolbar will refer
to the frame's window. That property cannot be changed after the document 
is created.

However, if any reference is made to 'window.toolbar' before the frameset is 
parsed, then window.toolbar == [Object Bar], and that value cannot be changed
after it the property has been added to Window.

In mozilla, for the properties toolbar, menubar, locationbar, personalbar, 
scrollbars, and statusbar, the value of these are all [Object BarProp], and 
cannot be changed at any time.

Comment 20

[John Morrison]

Attachment: mini test case for these seven properties (toolbar, menubar, etc.)

Comment 21

[Peter Trudelle]

nsbeta3+, P4 for M18, Should fix, but can't guarantee we'll get to it.  Any 
other top sites known to have a problem with this?

Comment 22

[leger]

PDT would like to see this fixed.  Trudelle, can you raise level?

Comment 23

[Peter Trudelle]

P3, but it would be good to know why this is important.

Comment 24

[Dan M]

  Alright. I won't be able to get to this for a week, so here comes a description 
of what needs to be done if someone wants to look at it.
  There were seven properties (all the BarProp objects: directories, locationbar, 
menubar, personalbar, scrollbars, statusbar, toolbar) added to the window JS 
object in Navigator 4 that need to be redefineable. Currently they're predefined 
in IDL (Window.idl), which makes them always hard-wired to specific lookup 
functions. (Most of them are defined "replaceable," but it turns out our 
implementation of replaceable still leaves them too hardwired to be replaced by a 
frame of that name, and there's the problem.)
  So, unfortunately, those properties need to be simply removed from Window.idl. 
That will orphan their seven predefined lookup methods in nsGlobalWindowImpl.cpp. 
Probably those orphaned methods should be left alone in the hope that they may 
one day be revived, and declared explicitly in nsGlobalWindowImpl.h.
  Then I think the task of finding these properties will fall to 
GlobalWindowImpl::GetProperty, SetProperty and Resolve. Resolve is the one that 
finds named frames. G/SetProperty, being generic methods, will need to kick in if 
one of those seven properties are requested, otherwise punting as they currently 
must. Resolve needs to be given first crack (that's up to the caller, of course, 
but somehow we need to be sure if there's a named frame, that's the thing that's 
found). Then SetProperty should store a JS value if requested, and GetProperty 
should return that JS value, or make up the appropriate BarProp if there is 
nothing defined.
  A bit sketchy, but I think that's a workable outline. Marking it helpwanted. I 
can get to this, barring emergencies, right before the nsbeta3 cutoff date. If 
anyone cares to jump in before that, well, bless you.

Comment 25

[Brendan Eich [:brendan]]

Dan: almost, but you must use Resolve for all lazy property definition, never 
GetProperty and SetProperty (or you'll create bugs such as bug 38215 and bug 
49213).  That also centralized the logic that orders frame vs. *bar names: do 
frame names trump *bar names?  Then search the frame kids in Resolve first.  
Else search the *bar names (probably should make a static table).

Is it possibly easier to make DOM idl's replaceable cooperate with generated 
resolve glue methods?  If so, do that -- it might help the bugs I cite above.  
Of course, DOM idlc is doomed to die some time later this year or early next, 
with a spanky conversion to XPIDL and XPConnect.

/be

Comment 26

[Brendan Eich [:brendan]]

Plus, Resolve is more efficient, as it runs only for properties not defined.  If 
you burn cycles testing for "toolbar", etc. in GetProperty and SetProperty, you 
will kill JS performance in DOM environments (see bug 43902).

So Resolve is the one true way, for maximum efficiency and for consolidated 
ordering logic.  Now if only DOM idlc would generate the logic in the glue code 
(after any GenericResolve call that did not define the property), translating 
replaceable attributes into a table of properties that the resolve glue-method 
defines.  Cc'ing vidur for comments.

/be

Comment 27

[vidur (gone)]

Fair enough. That's what "replaceable" should have been in the first place. I'm 
taking the bug.

Comment 28

[Brendan Eich [:brendan]]

ekrock, pdt: since you gave the other notorious DOM level 0 compatibility bugs
to do with JS 'with' statements and certain DOM objects and properties (see bug
38215 and bug 49112) fixable priority, it seems to me you should do the same
here.  Or else revoke those bugs' priority and make jst back out the fixes he
committed (kidding, don't do that!).

/be

Comment 29

[ekrock's old account (dead)]

Increasing priority to P2. This is high profile backwards compatibility. Not
only are we already seeing this on visto.com, but the fundamental problem
(namespace collision of our Mozilla properties with legacy Nav4 properties) is
likely to affect numerous other sites and cause existing content to fail to
function correctly.

Comment 30

[Johnny Stenback (:jst)]

Attachment: Proposed fix (complete diff -u)

Comment 31

[Johnny Stenback (:jst)]

Attachment: diff -w excluding changes to the generated files

Comment 32

[Johnny Stenback (:jst)]

The proposed fix makes IDLC generate code that makes 'replaceable' properties be
lazily defined, and makes replaceable properties not shadow child frames with
colliding names (window.toolbar, for instance). This patch brings us a big step
closer to being backwards compatibile with 4.x.

Reassigning to me.

Comment 33

[Brendan Eich [:brendan]]

jst: you need to set *aDidResolved based on the resolved local passed to 
JS_ResolveStandardClass:

+  *aDidDefineProperty = PR_FALSE;
+
   if (JSVAL_IS_STRING(aID)) {
>>>  JSBool resolved;    <<< if this comes back true, set *aDidResolved from it
     JSString *str;

in GlobalWindowImpl::Resolve.

From nsJSUtils::nsGenericResolve:

+    if (object) {
+      PRBool didDefineProperty = PR_FALSE;
+
+      object->Resolve(aContext, aObj, aId, &didDefineProperty);
+
+      if (!didDefineProperty && JSVAL_IS_STRING(aId) && aLazyPropSpec) {

Why not test object->Resolve's r.v. and if false, fail right away?  Seems wrong 
to do otherwise.

Soon after, same method:

+        JSString *str = JSVAL_TO_STRING(aId);
+
+        nsCAutoString name;
+        name.AssignWithConversion(NS_REINTERPRET_CAST(const PRUnichar*, 
::JS_GetStringChars(str)));

I would avoid nsCAutoString and call JS_GetStringBytes, then make sure that you 
didn't chop any Unicode characters with non-zero high bytes down to different 
ASCII/ISO-Latin-1 characters:

+        PRInt32 i;
+        JSString *str = JSVAL_TO_STRING(aId);
+        const unsigned char *bytes = (unsigned char *)JS_GetStringBytes(str);
+        const jschar *chars = JS_GetStringChars(str);
+        for (i = 0; bytes[i]; i++) {
+            if (jschar(bytes[i]) != chars[i])
+                return JS_TRUE;
+        }
+
+        for (i = 0; aLazyPropSpec[i].name; i++) {
+          if (strcmp(aLazyPropSpec[i].name, bytes) == 0) {
+            return ::JS_DefineUCProperty(aContext, aObj,
+                                         ::JS_GetStringChars(str),
+                                         ::JS_GetStringLength(str),
+                                         JSVAL_VOID,
+                                         aLazyPropSpec[i].getter,
+                                         aLazyPropSpec[i].setter,
+                                         aLazyPropSpec[i].flags);
           }
         }

I also fixed the ::JS_DefineProperty return value to be propagated.

I skimmed the idlc (JSStubGen) changes, they looked ok to me and if the code 
compiles and works, a=[e-mail].

/be

Comment 34

[Brendan Eich [:brendan]]

I wrote:

>I would avoid nsCAutoString and call JS_GetStringBytes, then make sure that you 
>didn't chop any Unicode characters with non-zero high bytes down to different 
>ASCII/ISO-Latin-1 characters:
>
>+        PRInt32 i;
>+        JSString *str = JSVAL_TO_STRING(aId);
>+        const unsigned char *bytes = (unsigned char *)JS_GetStringBytes(str);
>+        const jschar *chars = JS_GetStringChars(str);
>+        for (i = 0; bytes[i]; i++) {
>+            if (jschar(bytes[i]) != chars[i])
>+                return JS_TRUE;
>+        }

but forgot to add the necessary (because in low memory situations, 
JS_GetStringBytes may return "");

+        if (size_t(i) != JS_GetStringLength(str))
+            return JS_TRUE;
+

Ok, I'm done!

/be

Comment 35

[Johnny Stenback (:jst)]

Ok, thanks for the input Brendan. I've incorporated your suggestions and I'll
attach a new diff for nsGlobalWindow.cpp and nsJSUtils.cpp. I made a few minor
modifications tho, in stead of using strcmp(), I used nsCRT::strcmp() since I've
seen the mac builds break when using strcmp() (if the right headers aren't
included, and I don't know what those would be) even if it builds on all other
platforms and I don't wanna risk breaking the tree now.

Comment 36

[Johnny Stenback (:jst)]

Attachment: Updated patch for nsGlobalWindow.cpp and nsJSUtils.cpp

Comment 37

[Johnny Stenback (:jst)]

Fix checked in.

Comment 38

[John Morrison]

Reopening. Some of the 7 properties were left behind in this fix. 

The properties:
      window.toolbar
      window.menubar
      window.locationbar
      window.personalbar
are now fully replaceable. (e.g., if 'foo' is the name of a <frame> then 
that frame may be referenced as window.foo; also, unlike 4.x you may now
directly set that property 'window.foo = 99;' (but window.frames['foo']
will still contain a reference to the actual frame, not the value 99.  
      
However, these properties are not replaceable (possibly just need to 
declare them in Window.idl).    
      window.scrollbars
      window.statusbar
      window.directories
They do not refer to a frame if a frame has that name, and cannot be set
to some other value.

Comment 39

[Peter Trudelle]

nsbeta3-

Comment 40

[Nisheeth Ranjan]

Futured, unless we identify problems on high profile sites that are affected by
this bug.  The workaround is to change the name of the JS object or frame so
that it doesn't conflict with the three remaining non-replaceable window properties.

Comment 41

[Brendan Eich [:brendan]]

Why are we preempting names that 4.x did not?  I bet donuts this will break real 
pages.  Isn't it trivial to make the remaining three bad boys replaceable?

/be

Comment 42

[Johnny Stenback (:jst)]

Attachment: Proposed fix for the last properties...

Comment 43

[Johnny Stenback (:jst)]

So I couldn't hold myself, I just hadto spend the two minutes it took to fix the
last properties. Brendan, could you give the last patch some r & a love?

PDT: The last patch might look big and scary but the change is really only the
tree line change in Window.idl, the rest of the changes are due to the
regeneration of the glue code, and we know that code works, it's been with us
for a loong time by now.

Comment 44

[Brendan Eich [:brendan]]

I hereby give r= or a=, vidur can do the other one.  PDT, if you took the last 
one, take this one -- it's not "too late" given the other stuff going into the 
branch.  This is a rum-dum regenerated glue code change.

/be

Comment 45

[Johnny Stenback (:jst)]

Thanks Brendan!

I did some more checkin to see if these properties are even used anywhere in
mozilla and they're not used anywhere, not in mozilla nor the commercial code so
there's no way fixing this can break anything inside of mozilla.

Comment 46

[ekrock's old account (dead)]

PDT, please accept the fix for RTM for reasons cited earlier. The only reason we 
temporarily minused it earlier was a rote, dogmatic application of current 
PDT rules without applying the rule of reason. Let's do a rational cost-benefit 
analysis on this one and accept the zero-risk patch (yes, I know no patch is 
truly zero risk ...) that improves backward compatibility.

Comment 47

[Johnny Stenback (:jst)]

Ok, Vidur says r&a=vidur.

Comment 48

[Nisheeth Ranjan]

Marking rtm+

Comment 49

[selmer (gone)]

Which are the high profile sites which are currently broken? 

Comment 50

[Nisheeth Ranjan]

Please see Eric Krock's comment earlier about how this is an extremely low risk
fix for backwards compatibility.  We don't, however, know of specific high
profile sites that break as a result of this bug.

I understand if, at this late stage in the ship cycle, you choose to rtm- this
bug.  At the same time, it seems that if engineers close to the bug feel that
this fix is virtually risk-free and improves the product, then it should be
allowed in.  I'll let the PDT make the final call on this.

Marking rtm+

Comment 51

[Peter Trudelle]

PDT says rtm-, 90% case is in hand, no known high profile sites are a problem,
this will have to do for rtm.

Comment 52

[Brendan Eich [:brendan]]

Can we get this into the trunk, please?  It has passed code review.

/be

Comment 53

[Johnny Stenback (:jst)]

I'm sorry about the delay, now this is finally checked in, marking FIXED.

Comment 54

[Chris Petersen]

Marking verified with the Feb 26th build under Windows ME.