355075 - Various (Strict) "Warning: reference to undefined property" regressions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Serge Gautherie (:sgautherie)]

Regressed between
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20060919 SeaMonkey/1.1b] (nightly) (W98SE)
and
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061001 SeaMonkey/1.1b] (nightly) (W98SE)

1. Start Browser.
2. Got to <https://bugzilla.mozilla.org/enter_bug.cgi?full=1>.
r.
[
Warning: reference to undefined property this.mProgressListeners
Source File: chrome://global/content/bindings/tabbrowser.xml
Line: 856
]

I get an error at almost every/any page load.

This line comes from "old"
[
Bugzilla Bug 297155
opening a new tab while loading first page in new window breaks throbber / status bar
]

***

Eventually, I'm not selecting "Component: Tabbed Browser"
because I believe the issue is more general (hence choosing "Core" rather than MAS/SeaMonkey),
as I've also seen repeated
[
Warning: reference to undefined property flavourSet.flavourTable
Source File: chrome://global/content/nsDragAndDrop.js
Line: 200
]
and
[
Warning: reference to undefined property this.mSessions
Source File: chrome://global/content/autocomplete.xml
Line: 634
]

Wild guess: could this be some kind of JS engine regression ?

Comment 1

[Serge Gautherie (:sgautherie)]

CC:
*Neil, as you made the Tabbrowser patch
*Brendan, in case it actually is a JS engine regression

I would also request blocking1.8.1=?,
but holding until we know at least if it is Core or SeaMonkey...

Comment 2

[[e-mail]]

Looks like a JS engine problem... tracing in Venkman shows that it breaks at the spurious warning but the JS then enumerates as expected.

Comment 3

[Serge Gautherie (:sgautherie)]

'blocking1.8.1=?':
Still a little early, but "Better safe than sorry" then, in case this should block the upcoming "FF2rc2".

Moving component from 'General' to 'JavaScript Engine', too.

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20060930 SeaMonkey/1.1b] (nightly) (W98SE)

No bug yet.

(It would seem we're lucky on this one:)
the regression happened between the following SM builds:
2006-09-30-13-mozilla1.8 and 2006-10-01-01-mozilla1.8.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Looks like a JS engine problem... tracing in Venkman shows that it breaks at
> the spurious warning but the JS then enumerates as expected.

Can you get a stack in the JS engine, as well as in the JS code?  It would be great to get a reduced testcase.

Does this bug bite any XUL app other than SeaMonkey?

/be

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: minimal testcase

I needed to set javascript.options.strict to see this bug - a 1.8 build from shows no warning, a build from today shows "Warning: reference to undefined property this.a".

Comment 7

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/2006100103 BonEcho/2.0] (nightly) (W98SE)

Same bug with FF v2:
[
Warning: reference to undefined property this.a
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=240871
Line: 8
]

Comment 8

[Igor Bukanov]

This is a "regression" introduced by the bug 354750. 

Now Object.prototype.__iterator__ does not exist and code relies on the fact that the default iterator should be used if __iterator__ property is not present. The only problem is that the code uses JS_GetMethodById which would report a warning when it tries to access a property that do not exist. I wrote "regression" since in fact the code just exposed a bug in JS_GetMethodById: it should not report warnings for non-existing methods as all its callers has to deal with that on its own.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> This is a "regression" introduced by the bug 354750. 
> 
> Now Object.prototype.__iterator__ does not exist and code relies on the fact
> that the default iterator should be used if __iterator__ property is not
> present. The only problem is that the code uses JS_GetMethodById which would
> report a warning when it tries to access a property that do not exist. I wrote
> "regression" since in fact the code just exposed a bug in JS_GetMethodById: it
> should not report warnings for non-existing methods as all its callers has to
> deal with that on its own.

Right -- it needs to use OBJ_LOOKUP_PROPERTY to test existence first.  Then it would either OBJ_GET_PROPERTY or an optimized form of same.  This shows lack of optimality in JSObjectOps, for sure.  But not worth fixing in a hurry now.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 240876 [details] [diff] [review]
fix

Fast testing confirmation needed.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 13

[Igor Bukanov]

The problem with fix v1:

@callisto~/m/trunk/mozilla/js/src> cat ~/m/files/y.js 
function f() {
  this.a = <><a/><b/></>
  var dummy;
  for (var b in this.a)
    dummy = b;
}

f();
@callisto~/m/trunk/mozilla/js/src> js -strict ~/m/files/x.js
LAZY: 0
/home/igor/m/files/x.js:4: strict warning: reference to undefined property this.a
/home/igor/m/files/x.js:4: strict warning: reference to undefined property this.a
/home/igor/m/files/x.js:4: strict warning: reference to undefined property this.a
@callisto~/m/trunk/mozilla/js/src> 

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment 240877 [details] [diff] fixes this bug for me.

Comment 15

[Brendan Eich [:brendan]]

Fix on trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.281; previous revision: 3.280
done

/be

Comment 16

[Mike Schroepfer]

Comment on attachment 240877 [details] [diff] [review]
fix, v2

Approved regression fix for RC2.

Comment 17

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.27; previous revision: 3.214.2.26
done

/be

Comment 18

[Bob Clary [:bc] (inactive)]

Checking in regress-355075-01.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-355075-01.js,v  <--  regress-35
5075-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/iterable/regress-355075-02.js,v
done
Checking in regress-355075-02.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-355075-02.js,v  <--  regress-35
5075-02.js
initial revision: 1.1

Comment 19

[shutdown]

With the patch applied, JS_GetMethodById() on XML objects no longer works.

Comment 20

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061002 SeaMonkey/1.1b] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH.


(In reply to comment #9)
> This shows lack of
> optimality in JSObjectOps, for sure.  But not worth fixing in a hurry now.

I leave it to you to open a followup bug, if necessary.


(In reply to comment #19)
> With the patch applied, JS_GetMethodById() on XML objects no longer works.

Should this bug be reopened, or another one filed ?

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #19)
> With the patch applied, JS_GetMethodById() on XML objects no longer works.

What testcase shows this, or is it only if the API is used to get at XML.prototype methods?  I should have seen the latter problem; too much last-minute hacking.  If it is the only problem, we can live with it for 1.8.1 and fix it in 1.8.1.1.  New bug is needed, since this bug's symptom is fixed, for both normal and E4X objects (as igor showed in comment 13).

/be

Comment 22

[shutdown]

(In reply to comment #21)
> (In reply to comment #19)
> > With the patch applied, JS_GetMethodById() on XML objects no longer works.
> What testcase shows this, or is it only if the API is used to get at

I filed bug 355145 with a testcase.

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > With the patch applied, JS_GetMethodById() on XML objects no longer works.
> > What testcase shows this, or is it only if the API is used to get at
> 
> I filed bug 355145 with a testcase.

Thanks.

/be

Comment 24

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

Comment 25

[Bob Clary [:bc] (inactive)]

this regressed on 20061009 1.8 after 20061007. filed Bug 356129.