contentAreaClick.js asks go nsIPref, but value is later clobbered

VERIFIED FIXED

Status

VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: spam, Assigned: bnesse)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
current CVS build, linux:

When middle-mouse clicking to open a new tab (background) i get this error in
console and no tab opens:

Error: pref.GetBoolPref is not a function
Source File: chrome://communicator/content/contentAreaClick.js Line: 188

Comment 1

17 years ago
This is part of the pref changes that landed today. 

The problem is that while |pref| is initially a '[xpconnect wrapped nsIPref]'
when 'contentAreaClick.js' is loaded, by the time that 'contentAreaClick()' is
called, |pref| has been reset to be a '[xpconnect wrapped (nsISupports,
nsIPrefBranch, nsIPrefBranchInternal)]'. And with the api changes for
nsIPrefBranch, |GetBoolPref| should be |getBoolPref|.

Sorry to say, but I think there are a whole bunch of these type of errors in
the tree right now, and lots of little bits of UI are broken.
(Reporter)

Comment 2

17 years ago
Lowercasing the G's did the trick - thanks :)
(Reporter)

Comment 3

17 years ago
saw Hyatt just checked in v 1.16 w lowercase - resolving as fixed 
mozilla/xpfe/communicator/resources/content/contentAreaClick.js
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 4

17 years ago
Thanks, R.K., although I'm going to reopen this bug (and gift it on blake).

The thing is that that file begins:

 42   var pref = null;
 43   pref = Components.classes["@mozilla.org/preferences;1"];
 44   pref = pref.getService();
 45   pref = pref.QueryInterface(Components.interfaces.nsIPref);

but by the time it actually uses |pref|, it isn't an nsIPref anymore, it's 
an nsIPrefBranch (likely bonked by navigator.js?).

(Is there a plan for eradicating nsIPref from the js/xul and the binary code?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 5

17 years ago
-> blake.
Assignee: hyatt → blakeross
Status: REOPENED → NEW
Summary: middle-mouse click to open new tab gives Error: pref.GetBoolPref is not a function → contentAreaClick.js asks go nsIPref, but value is later clobbered
(Reporter)

Comment 6

17 years ago
oops.. i saw no errors in js console, and it works like a dream now, thus the
"fixed". Are you saying there's a leak there?
(Assignee)

Comment 7

17 years ago
>but by the time it actually uses |pref|, it isn't an nsIPref anymore, it's 
>an nsIPrefBranch (likely bonked by navigator.js?).
Oh, that's special... globals that replace other globals... :(

The right fix here will probably be to move this over to prefservice/branch. I
can do that.

>(Is there a plan for eradicating nsIPref from the js/xul and the binary code?)
Yes, unfortunately it is tedious and has been a fairly low priority.
(Assignee)

Comment 8

17 years ago
->me
Assignee: blakeross → bnesse
(Assignee)

Comment 9

17 years ago
Created attachment 54736 [details] [diff] [review]
patch to convert nsIPref -> nsIPrefBranch
(Assignee)

Comment 10

17 years ago
The attached patch will create "pref" in a way that is compatible with the one 
created in navigator.js.

Comment 11

17 years ago
Does contentAreaClick.js need to initialize |pref| at all?
(Assignee)

Comment 12

17 years ago
I'm going through that whole directory right now. There is some "sharing" of
global(s) going on in there. A number of the files declare pref (or prefs) and
initialize it (them). Other files (contentAreaUtils.js for instance) assume that
it has already been initialized.
(Assignee)

Comment 13

17 years ago
Created attachment 54775 [details] [diff] [review]
More complete patch that switches all js files in /content/ to nsIPrefBranch
(Reporter)

Comment 14

17 years ago
jst@netscape.com just checked in a diff against contentAreaClick.js v 1.16
So the diff here should be made against new v 1.17 i guess, depending on what
jst checked in..

Comment 15

17 years ago
*** Bug 106215 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

17 years ago
jst's changes are unrelated, but I will update the patch...
(Assignee)

Comment 17

17 years ago
Created attachment 54878 [details] [diff] [review]
Updated for jst's change

Comment 18

17 years ago
*** Bug 106245 has been marked as a duplicate of this bug. ***

Comment 19

17 years ago
*** Bug 106752 has been marked as a duplicate of this bug. ***

Comment 20

17 years ago
This isn't really Linux only, right?
(Assignee)

Updated

17 years ago
OS: Linux → All
Hardware: PC → All
*** Bug 106782 has been marked as a duplicate of this bug. ***

Comment 22

17 years ago
I just got a bug duped to this one. My problem is that middle clicking on a mail
link does not open a new window with the link , so my problem is not with tabs.
Anyway, i have today's build and i still cant do this.
Would somebody else take a look at  Bug 106782 and tell me if this will be fixed
by this bug?

(Assignee)

Comment 23

17 years ago
There is a pretty good chance that there is a relation. It's not entirely tab
specific, it's more about preferences being confused by multiple pref objects
(with different types.)

Comment 24

17 years ago
I have not changed my prefs settings in any way like dark wrote in my duped bug,
therefore, it goes open again. I will be gladly to make it dup if when this one
is resolved mine does too

Comment 25

17 years ago
*** Bug 106885 has been marked as a duplicate of this bug. ***

Comment 26

17 years ago
*** Bug 106935 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 27

17 years ago
Created attachment 55286 [details] [diff] [review]
Patch to remove all nsIPref references from js & xul files.
(Assignee)

Comment 28

17 years ago
The attached patch should eliminate all possible conflicts between nsIPref and 
nsIPrefBranch in the mozilla tree.

Comment 29

17 years ago
Comment on attachment 55286 [details] [diff] [review]
Patch to remove all nsIPref references from js & xul files.

It looks okay. r=jag
Attachment #55286 - Flags: review+

Comment 30

17 years ago
Comment on attachment 55286 [details] [diff] [review]
Patch to remove all nsIPref references from js & xul files.

whoo hoo! go brian
sr=alecf
Attachment #55286 - Flags: superreview+
(Assignee)

Comment 31

17 years ago
Patch checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 32

17 years ago
Ugh, I completely missed that getComplexValue doesn't return a wstring, but a
nsIPrefLocalizedString or nsISupportsWString. This means that in certain cases
you'll need to call |.data| or |.toString()| explicitely to get at the inner
string that you can then manipulate as a js string (e.g. call |.toLowerCase()|).
Checked in one "bustage" fix already[0]. bnesse, could you go over your patch
and make sure there aren't similar cases lurking?

[0]
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/sidebar/resources&command=DIFF_FRAMESET&file=sidebarOverlay.js&rev1=1.83&rev2=1.84&root=/cvsroot

Comment 33

17 years ago
See bug 107104 for more "bustage".

Updated

17 years ago
Blocks: 106782
(Assignee)

Comment 34

17 years ago
I'll review all of the getComplexValue calls now in the tree.
(Assignee)

Comment 35

17 years ago
Created attachment 55814 [details] [diff] [review]
Patch to fix getComplexValue calls (and clean up some warnings)

Comment 36

17 years ago
Comment on attachment 55814 [details] [diff] [review]
Patch to fix getComplexValue calls (and clean up some warnings)

sr=alecf
Attachment #55814 - Flags: superreview+

Comment 37

17 years ago
Comment on attachment 55814 [details] [diff] [review]
Patch to fix getComplexValue calls (and clean up some warnings)

Thanks Brian.  r=sgehani
Attachment #55814 - Flags: review+
(Assignee)

Comment 38

17 years ago
Checked in.
QA Contact: blakeross → sairuh
i don't see this error in the js console [2001.11.28.08-comm, linux] when i
middle-mouse click a link to load in a new tab in the background. marking vrfy'd.
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.