178062 - [FIXr]JS console not showing any JavaScript strict warnings

Status: VERIFIED FIXED

(Core Graveyard :: Error Console, defect, P1)

Description

[Steve Chapel]

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.

Comment 1

[Steve Chapel]

Attachment: Testcase that calls a function that does not always return a value

Comment 2

[Boris Zbarsky [:bzbarsky]]

Confirming, with strict warnings turned _on_.  Build 2002-11-01-04 on Linux.

Comment 3

[Phil Schwartau]

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 -

Comment 4

[Phil Schwartau]

Attachment: JS shell version of testcase

Comment 5

[Phil Schwartau]

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 -

Comment 6

[Phil Schwartau]

Attachment: HTML testcase that should generate 5 distinct JS strict warnings

Comment 7

[Boris Zbarsky [:bzbarsky]]

upgrading severity accordingly.....

Comment 8

[Phil Schwartau]

Attachment: JS shell version of the above testcase

Comment 9

[Phil Schwartau]

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

Comment 10

[Phil Schwartau]

From the duplicate bug 178132, reported by [e-mail]:

> 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.

Comment 11

[Phil Schwartau]

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

[Phil Schwartau]

This doesn't look to be a JS Engine bug; reassigning to
the JavaScript Console component and to danm for a look -

Comment 13

[Brendan Eich [:brendan]]

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

[Alec Flett]

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. 

Comment 15

[Boris Zbarsky [:bzbarsky]]

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....

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

[Dan M]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[Dan M]

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!)

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

[Dan M]

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

[Dan M]

  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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Oh, well

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

Comment 24

[Alec Flett]

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

[Alec Flett]

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 :)

Comment 26

[Boris Zbarsky [:bzbarsky]]

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 27

[Brendan Eich [:brendan]]

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

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

/be

Comment 28

[Asa Dotzler [:asa]]

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

a=asa for checkin to the 1.2 branch (on behalf of drivers)

Comment 29

[Boris Zbarsky [:bzbarsky]]

I should take this or something...  Waiting for the branch to reopen to check
this in, though...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed, trunk and 1.2 branch.

Comment 31

[jag (Peter Annema)]

New bug# for the preferred (comment 20, comment 24, comment 26) fix?

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

[Phil Schwartau]

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 -