js changes mean directory listings get wrong context menu

RESOLVED FIXED in mozilla0.8



17 years ago
10 years ago


(Reporter: Bradley Baetz, Assigned: Bill Law)



Firefox Tracking Flags

(Not tracked)




(3 attachments)



17 years ago
While regression testing gopher, I discovered that the javascript changes last
week to cause wrapped xpconnect objects to be printed with their address broke
the context menu for the directory viewer.

nsContextMenu.js at about line 276 did a string equality with the value, and
this now fails. However, even when I change this comparision to just test a
substring, the third conditional (!window._content.HTTPIndex.constructor) fails.
Since I don't know what that conditional is for, I don't know how to fix it. I'm
presuming that it was the same JS change - I haven't tried backing it out to

You can see this by bringing up the context menu. Open link in new window should
be an option, but its not. This was confirmed on IRC, so its not just my hacked

Comment 1

17 years ago
That nsContextMenu.js code is making some bad assumptions and needs to be fixed.

 if (window._content.HTTPIndex == "[xpconnect wrapped nsIHTTPIndex]...
---  can be changed to ---
 if(window._content.HTTPIndex instanceof Components.interfaces.nsIHTTPIndex...

I'm not clear why this code would want to be checking for the 'constructor' 
property. Any JSObject that has Object.proto is into prototype chain would 
have that property. My fix to bug 64111 included correcting the oversight that 
the JSObject part of xpconnect wrapped natives did not have a __proto__. Now 
they do (like most every other JSObject). The DOM conversion to use xpconnect 
will require this soon enough anyway. Was this check there only to try to verify 
that you were looking at an xpconnect wrapped native?
Assignee: jband → blakeross

Comment 2

17 years ago
Tracing back through bonsai (The file has been moved), this was checked in by
law@netscape.com for bug 32357.

> Was this check there only to try to verify that you were looking at an
> xpconnect wrapped native?

Comment 3

17 years ago
The code in question was just a reasonable attempt to verify that the content
area was a ftp/file directory listing internally generated by the browser rather
than some random web page.  I don't recall the exact reasoning but I think it
was to try to avoid being spoofed by a web page with conventional
(non-xpconnect) JS.

There's similar code elsewhere to handle charsets for such directory listings, BTW.

Both of those need to change now.  It sounds like the "instanceOf" operator is
the right way to do it.

Comment 4

17 years ago
> There's similar code elsewhere to handle charsets for such directory listings,

Where? Isn't everything in ASCII? They're uris, and escaped/unescaped. If
charsets are a problem, I'll test my gopher stuff against it if someone can
point me to something which triggers that code.

So the three tests can be replaced with the single instanceOf test? Patch later

Comment 5

17 years ago

That's to properly deal with file names that aren't plain ASCII, or so I believe.

I think the instanceOf test seems to be the right solution.  That's what the
bogus code was trying to do.

Comment 6

17 years ago
Why is this mine?  I didn't write that...
Assignee: blakeross → pinkerton

Comment 7

17 years ago
I assigned it to you because cvs blame showed you as the person who checked in 
the lines to nsContextMenu.js. I now see in the cvs log that you were 
'extract'ing it from somewhere else. It's fine with me if you reassign this. 
Maybe Bill Law will volunteer to fix the cases - he seems to have a handle on 

Comment 8

17 years ago
I'll take it.
Assignee: pinkerton → law
Keywords: nsbeta1+
Priority: -- → P1
Target Milestone: --- → mozilla0.8

Comment 9

17 years ago
I did the patch, then checked my mail...

Attached, and it fixed the problem for ftp, and html still works. I haven't
tested on non-ASCII file names - anyone?
Keywords: patch

Comment 10

17 years ago
Created attachment 23879 [details] [diff] [review]

Comment 11

17 years ago
Created attachment 23914 [details] [diff] [review]
jag's patch caused a cvs conflict

Comment 12

17 years ago
It looks like a strict warning will still be thrown if _content.HTTPIndex is 
null; you might want to keep the |"HTTPIndex" in _content| check.  Also, why 
the window._content/_content inconsistency?

Comment 13

17 years ago
Created attachment 24006 [details] [diff] [review]
r=blake with these changes

Comment 14

17 years ago
jband, sr=?

Comment 16

17 years ago
fix checked in.
Last Resolved: 17 years ago
Resolution: --- → FIXED


10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.