[FIXr]JS console not showing any JavaScript strict warnings

VERIFIED FIXED in mozilla1.3alpha

Status

Core Graveyard
Error Console
P1
major
VERIFIED FIXED
16 years ago
8 years ago

People

(Reporter: Steve Chapel, Assigned: bz)

Tracking

({regression, testcase})

Trunk
mozilla1.3alpha
regression, testcase

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: |javascript.options.strict|, |javascript.options.werror| both broken)

Attachments

(5 attachments)

(Reporter)

Description

16 years ago
Reproducible: Always

Steps to reproduce:
1. Execute any JS function that returns a value on some returns and does not
return a value on other returns.

Expected Results: The JavaScript console contains a warning about the function
not always returning a value.

Actual Results: The JavaScript console does not contain a warning about the
function.

I've tested this with build 2002110204 on Windows 98.
(Reporter)

Comment 1

16 years ago
Created attachment 104947 [details]
Testcase that calls a function that does not always return a value
(Reporter)

Updated

16 years ago
Keywords: mozilla1.3, regression, testcase
Confirming, with strict warnings turned _on_.  Build 2002-11-01-04 on Linux.

Comment 3

16 years ago
Here is a JS shell version of the testcase:

function sometimes(x)
{
  if (x > 0)
    return x;
}

print(sometimes(1));
print(sometimes(0));


If I run this in the JS shell via the -f option, I DO get a warning
if I have the strict option -s turned on:


[ ] ./js -s -f ../../tests/test/test/178062.js
178062.js:9: strict warning: function sometimes does not always return a value:
178062.js:9: strict warning: }
178062.js:9: strict warning: ^
1
undefined


But if I start the JS shell independently, with the -s option set,
and load the testcase interactively, I do not get a warning:

[ ] ./js -s
js> load('../../tests/test/test/178062.js');
1
undefined


I do not know if this bug is one for the JS Engine or for the 
embedder, to capture the warning JS may already be providing.

cc'ing Brendan, Mike, Roger -
Assignee: rogerl → khanson

Comment 4

16 years ago
Created attachment 104962 [details]
JS shell version of testcase

Comment 5

16 years ago
Using Mozilla trunk binary 2002110408 on WinNT.

I am not seeing ANY JS strict warnings in the browser, even when I have
"Show JavaScript strict warnings" checked in Edit > Preferences > Debug.

Will attach an HTML testcase below that should generate five different
JS strict warnings in the JS Console, but is generating none -

Updated

16 years ago
Summary: JS console should warn when function does not always return a value → JS console not showing any JavaScript strict warnings

Comment 6

16 years ago
Created attachment 105103 [details]
HTML testcase that should generate 5 distinct JS strict warnings
upgrading severity accordingly.....
Severity: normal → major

Comment 8

16 years ago
Created attachment 105104 [details]
JS shell version of the above testcase

Comment 9

16 years ago
*** Bug 178132 has been marked as a duplicate of this bug. ***

Comment 10

16 years ago
From the duplicate bug 178132, reported by bht@actrix.gen.nz:

> Moz 1.2b
> I think since 1.1, the following options are ignored:

> user_pref("javascript.options.strict", true);
> user_pref("javascript.options.werror", false);

> When I use Netscape 7.0 with the same profile then these work fine.
Whiteboard: |javascript.options.strict|, |javascript.options.werror| both broken

Comment 11

16 years ago
FWIW, my Mozilla trunk binary from 2002-07-01 shows all the strict
warnings, whereas my trunk binary from 2002-08-05 does not.

So a regression between those dates. Again, not sure if this is due to
a bug in JS Engine or in the embedding of JS Engine. I do know that
the current JS shell shows all the strict warnings if I load testcases
via the -s and -f options as described in Comment #3 above -

Comment 12

16 years ago
This doesn't look to be a JS Engine bug; reassigning to
the JavaScript Console component and to danm for a look -
Assignee: khanson → danm
Component: JavaScript Engine → JavaScript Console
Did the pref code change incompatibly?  The nsJSContext::JSOptionChangedCallback
in dom/src/base/nsJSEnvironment.cpp is registered for "javascript.options."
using nsIPref::RegisterCallback.

/be

Comment 14

16 years ago
if the pref code changed and broke this, then it is a bug - but nothing has
changed in that area in quite a while, as far as I know. 
This works in Linux build 2002-07-24-21 and is broken in 2002-07-25-21.  I'm
still coaxing the checkin list out of Bonsai....
More fun.  The warnings "work" in a debug build (requires a reload after the
initial load), but not opt.

The culprit is rev 1.178 of nsJSEnvironment.cpp.  If I switch it back to using
the CID, the warnings return... Why is do_GetService() failing with the contract
id?  

Comment 17

16 years ago
Boris: I see warnings in debug builds too. But I can't reproduce your results
with restoring warnings by going back to rev 1.177's kPrefServiceCID.
Hm... odd....

as a note, with the contractid the error I get is NS_ERROR_NO_INTERFACE... The
failing QI is the one in nsComponentManagerImpl::GetServiceByContractID

Comment 19

16 years ago
Scratch comment 17. My mistake. Confirming, then: kPrefServiceCID does restore
warnings to the console in my Windows trunk build (if I remember to turn on the
strict pref!)
Oh, boy.

The old code:

static NS_DEFINE_IID(kPrefServiceCID, NS_PREF_CID);                             

The new code:

nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));     

If you look at
http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefsFactory.cpp
you see that these CID and contractid belong to _different_ classes and that the
one that implements NS_PREFSERVICE_CONTRACTID does _not_ implement nsIPref. 
alecf, this is all yours.  :)

So possible fixes:

1)  Change to NS_PREF_CONTRACTID
2)  Change to using nsIPrefService instead of nsIPref (preferred, since nsIPref
    is deprecated)

As long as you're at this, fix
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapProtocol.cpp#7102
as well?

Comment 21

16 years ago
Yes, that's the problem. Nice, Boris! Slightly off topic, I've run into this
problem before, where the debug build has a curious tolerance for slightly wrong
CIDs that the optimized build lacks. Anybody know why?

Comment 22

16 years ago
  Oh. Strict is on by default in debug builds, isn't it? Ignore me once again.
My brain and I, we're taking the rest of the night off. Still, I swear I'm not
making that up about the CIDs.
  Maybe I shouldn't swear.
Created attachment 105154 [details] [diff] [review]
Oh, well

I can't get AddObserver() from inside the constructor to not crash, so let's go
with the simple fix....

Comment 24

16 years ago
I just want to voice my preference for switching to nsIPrefService - its the
right way to go. In addition, you can use nsIPrefBranchInternal to get to the
c-style callbacks mechanism

Comment 25

16 years ago
Comment on attachment 105154 [details] [diff] [review]
Oh, well

however, I just noticed this patch, and it sure is easy. sr=alecf for the
simple fix shown here :)
Attachment #105154 - Flags: superreview+
I agree that we should try to switch to nsIPrefService... but I'd like to land
that simple patch for 1.2, if possible.

Like I said, I tried adding the JSContext as a pref observer in the constructor
and that failed miserably (we're talking crash with gdb just being able to tell
me that Moz had jumped to memory gdb could not read and hence no stack).  I see
no way to get the old-style callbacks from nsIPrefBranchInternal...

So anyone?  r= on that patch?  ;)
Comment on attachment 105154 [details] [diff] [review]
Oh, well

Thanks, bz.  Can we get this into 1.2final?  Pls. mail drivers.

/be
Attachment #105154 - Flags: review+

Comment 28

16 years ago
Comment on attachment 105154 [details] [diff] [review]
Oh, well

a=asa for checkin to the 1.2 branch (on behalf of drivers)
Attachment #105154 - Flags: approval+
I should take this or something...  Waiting for the branch to reopen to check
this in, though...
Assignee: danm → bzbarsky
Priority: -- → P1
Summary: JS console not showing any JavaScript strict warnings → [FIXr]JS console not showing any JavaScript strict warnings
Target Milestone: --- → mozilla1.3alpha
Fixed, trunk and 1.2 branch.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 31

16 years ago
New bug# for the preferred (comment 20, comment 24, comment 26) fix?
None so far... wanna file it?  ;)

We should actually clean up usage of this nsIPref thing in general... it's only
been deprecated for about 2 years....

Comment 33

16 years ago
Verified FIXED using trunk and branch builds 20021117xx on Windows.
When the |javascript.options.strict| pref is set, the HTML testcase
in Comment #6 now generates all 5 strict warnings in the JS Console -
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey

Updated

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