33650 - Frame named "content" can't be accessed via parent.content

Status: VERIFIED FIXED

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

Description

[Eric Pollmann]

Clicking on a menu link in the left frame causes the toplevel window to be
updated.  This is because the frame that is supposed to be updated is named
"content".  The frame is updated like this:

parent.content.location="http://...

This works in Nav, but doesn't work in Gecko.

Comment 1

[Eric Pollmann]

Attachment: inner frame

Comment 2

[Eric Pollmann]

Attachment: Reduced test case

Comment 3

[David Krause]

Here is a less technical description of this bug.

Overview Description:
Url causes menu frames to be lost.

Steps to Reproduce:
1) Goto http://www.securityfocus.com/
2) Click on Forums -> Mailing Lists -> Bugtraq
3) Observe that the menu disappears

Actual Results:
The right window opens fully and the menu disappears.

Expected Results:
The menu should still be there with three submenus.

Reproducibility:
Everytime.

Build Date & Platform Bug Found:
Mozilla/5.0 (X11; U; Linux 2.2.12-20 i686; en-US; m14)
Build 2000040723

Additional Builds and Platforms Tested On:
win98

Additional Information:
Works fine in netscape 4.x and opera.

Comment 4

[David Krause]

Verified that attachment (id=7046) is a reduced test case.
2000040723 Linux i686

Comment 5

[travis]

So the interesting thing here is that GlobalWindowImpl::Resolve is called a 
number of times with some very very odd class names, but never with 
the "content" name that should be resolved from the parent.  Shaver, hadn't we 
found this ::Resolve function to before being called by some very odd names 
that shouldn't be being called.  What was our fix there?

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The fixes were to use mechanisms for class initialization that didn't result in
a full-scope-chain walk the first time through.

I recall that XPConnect and the DOM were implicated, and I dimly recall that
they were actually fixed.  What odd names are you seeing go through
GlobalWindowImpl::Resolve now?

Comment 7

[travis]

Resolve is called with such exciting class names as:
1.) HTMLDocument originating from 
http://lxr.mozilla.org/seamonkey/source/dom/src/html/nsJSHTMLDocument.cpp#1197
2.) HTMLHtmlElement originating from
http://lxr.mozilla.org/seamonkey/source/dom/src/html/nsJSHTMLHtmlElement.cpp#234
3.) HTMLBodyElement originating from
http://lxr.mozilla.org/seamonkey/source/dom/src/html/nsJSHTMLBodyElement.cpp#364
4.) HTMLAnchorElement originating from
http://lxr.mozilla.org/seamonkey/source/dom/src/html/nsJSHTMLAnchorElement.cpp#7
68
5.) valueOf from some XPConnect code.
6.) toString from some XPConnect code

Comment 8

[travis]

Ok, I just went and talked to brendan about this, and basically the problem 
with the content thing is that it has been defined in domidl as an attribute.  
So the problem is that .content was added to allow getting to the primary 
content shell of a window.  This however breaks sites that name their frames 
content.  So we need to remove this notation capability to get to the primary 
frame.  Flipping to hyatt since I think he created all this fun. :)

Comment 9

[Brendan Eich [:brendan]]

To recap my conversation with travis: Window.idl is wrongly predefining content
as an attribute, which predefines a JS property and preempts that name from
being resolved to a user-defined HTML frame.  The DOM idl attribute should be
removed, and global window resolve should try for the _content target iff it
doesn't find a user-defined frame named "content".

Travis, is your six-item-list an exhaustive list?  1-4 are due to the way
DOM-idlc-generated code bootstraps its class definition, and should happen once
per class per document load in a window or frame.  5 and 6 seem like bugs,
probably in JS code, where someone is passing window to an XPConnected method
that expects a string or possibly an int or other numeric type.  Jband?

/be

Comment 10

[Dan M]

What Brendan suggests has been done. This code, for instance:

<?xml version="1.0"?>
<!DOCTYPE window>
<window
    xmlns:html="http://www.w3.org/TR/REC-html40"
    xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
    title = "replaceable content test">
  <html:script>
    function runtest() {
      dump("window.content begins = '"+window.content+"'\n");
      window.content=16;
      dump("window.content set to integer, now = '"+window.content+"'\n");
    }
  </html:script>
  <html:button onclick="runtest()">Test</html:button>
</window>

when the "Test" button is pressed, prints this:

window.content begins = '[object Window]'
window.content set to integer, now = '16'

This works because the DOM Window "content" attribute is defined as readonly 
replaceable. JS returns the predefined attribute only until other JS has changed 
it to something else; JS then treats it as a user variable. This is working 
today: I just ran this test. It must be something else that's causing this bug.

Comment 11

[John Bandhauer]

Maybe I'm doing something wrong, but I'm not seeing the lookups that travis 
mentioned. I am seeing all the lookups associated with init'ing the JS classes 
as the JSContext is populated with new stuff. I breaking at 
nsJSUtils::nsGlobalResolve after clicking on the indicated links in the pages 
mentioned in the bug report.

The proto trick that shaver mentioned to xpconnect is at:
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednativecla
ss.cpp#1242

I *don't* see a resove associated with it.

I do see a search for the costructor for this JS class I init:
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.c
pp#1087

That can be fixed. 

And, I certainly should fix this rather naked looking call to create instances 
of that object (used to hold 'out' params when native code calls JS code).
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.c
pp#1106

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

danm: your test case doesn't seem to include any interaction with frame naming,
which is at the core of this bug, I think.

The accessors for window.[[property]] need to check first to see if there's a
frame named by [[property]], in which case they should return a reference to
that frame, rather than the DOM1+ value.

Comment 13

[Dan M]

Ah. I can see where the frame lookup code would be a third path, not covered by 
the "replaceable" attribute. Damn. That's disappointing.

Comment 14

[travis]

No my list was not exhaustive.  Basically I just broughtup the attached test 
case and then set the breakpoint on GlobalWindowImpl::Resolve and then watched 
what came through.

Comment 15

[travis]

Ok, we just talked about this a little bit ago and basically we don't want to 
lose the functionality, but we want to make this test case work and make these 
namings consistent with the targets such as "_content", "_content_mail" etc.  
So basically we will add a window.getContent(in string).  This in string will 
be "_content" or one of the others.  null passed will basically do what 
window.content does now which is to return the primary content area within the 
current window.  I don't have the time to implement this right now, but if 
anyone else does, I am more than happy to help.  Otherwise this is on the back 
back burner for a while.

Comment 16

[Johnny Stenback (:jst)]

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

Comment 17

[Nathan Rogers]

travis, when you say you plan to add a window.getContent(in string), do you 
intend for this to be the only way to access the attribute?  For my project, I 
need the ability to set the content attribute to a different frame, not just 
read it.  I just hope that functionality doesn't go away.

Comment 18

[David Hyatt]

woo

Comment 19

[John Morrison]

Since this bug is set as blocker for 31378,35726 and those are already nominated
nsbeta2, then adding nsbeta2 to this one (effectively either a resolution to 
the window.content && frames problem goes in beta2, or they all get pushed out).

Comment 20

[Peter Trudelle]

[nsbeta2+]

Comment 21

[Dan M]

  It wasn't possible to double-team the use of window.content; returning our 
meaning unless it had been overridden by HTML. And Travis' suggestion of adding a 
getContent method stuck in our collective craw. As did every other possible 
solution. I finally went with a name change: content has become _content.
  Of course if someone out there is using HTML with frames named _content, we'll 
have the same problem all over again. But there's a precedent for thinking of 
names beginning with an underscore as reserved, so I'm optimistic. In the end, 
because of the importance of our content property to XUL, we'll just have to keep 
changing the property's name to something that no one has ever used before.
  At least there 's no longer a problem with _this_ page.

Comment 22

[Chris Petersen]

Fixed in the June 26th build (200062608).

Comment 23

[Simon Fraser [no longer active]]

Yuck