756572 - /suite/browser/test/browser_popupNotification.js reports 3 "JavaScript strict warning"

Status: NEW

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337348393.1337352489.23433.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 06:39:53
{
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #11] running test
JavaScript strict warning: chrome://communicator/content/bindings/notification.xml, line 2608: reference to undefined property this.notification.options.value
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #11] popup showing
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.value" {file: "chrome://communicator/content/bindings/notification.xml" line: 2608}]
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #11] popup shown

TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #19] running test
JavaScript strict warning: chrome://global/content/bindings/popup.xml, line 0: reference to undefined property this.popupBoxObject.popupState
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #19] popup showing
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property this.popupBoxObject.popupState" {file: "chrome://global/content/bindings/popup.xml" line: 0}]
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #19] popup shown
}

NB: Firefox doesn't report these warnings.

Comment 1

[[e-mail]]

(In reply to Serge Gautherie from comment #0)
Neither of these warnings should be possible:

> JavaScript strict warning:
> chrome://communicator/content/bindings/notification.xml, line 2608:
> reference to undefined property this.notification.options.value
This object is created as part of showGeolocationPrompt, but it is always created with both value and href properties. (I assume the href is there because there is no strict warning for it...)

> JavaScript strict warning: chrome://global/content/bindings/popup.xml, line
> 0: reference to undefined property this.popupBoxObject.popupState
This is even weirder. this.popupBoxObject returns an nsIPopupBoxObject which always provides a popupState attribute. So it can never be undefined...

Comment 2

[Philip Chee]

Is this something to do with __exposed_props__ ?

Comment 3

[Serge Gautherie (:sgautherie)]

Reproducible locally, with Opt build: (and "javascript.options.strict = true")

[Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Firefox/15.0a1 SeaMonkey/2.12a1] (sgautherie.bz#free.fr-67dbc42bd72d/try-comm-central-win32)
{
TEST-KNOWN-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | panel should be open
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property this.mBrowser" {file: "chrome://navigator/content/tabbrowser.xml" line: 1062}]
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #5] running test

TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #11] popup showing
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.value" {file: "chrome://communicator/content/bindings/notification.xml" line: 2627}]
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #11] popup shown

TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #19] popup showing
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | Console message: [JavaScript Warning: "ReferenceError: reference to undefined property this.popupBoxObject.popupState" {file: "chrome://global/content/bindings/popup.xml" line: 0}]
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #19] popup shown
}

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> TEST-INFO |
> chrome://mochitests/content/browser/suite/browser/test/
> browser_popupNotification.js | Console message: [JavaScript Warning:
> "ReferenceError: reference to undefined property this.mBrowser" {file:
> "chrome://navigator/content/tabbrowser.xml" line: 1062}]

Fwiw,

Firefox:
http://hg.mozilla.org/mozilla-central/rev/1e70fba7e663
   2.460 +              this._callProgressListeners(null, "onUpdateCurrentBrowser",

SeaMonkey:
http://hg.mozilla.org/comm-central/rev/afdde738a668
   1.187 +            this._callProgressListeners(this.mBrowser, "onProgressChange",
   1.205 +            this._callProgressListeners(this.mBrowser, "onUpdateCurrentBrowser",

Should SM be using 'null' there too?

Comment 5

[[e-mail]]

Good catch! The mTabProgressListener needs to pass its mBrowser, and setIcon needs to pass the tab's browser, but updateCurrentBrowser can just pass null.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.mBrowser"| in updateCurrentBrowser() [Checked in: Comments 9 and 23]

Per comment 5.
This fixes the 1st of the 3 warnings.
This code was added by bug 583317.

[Approval Request Comment]
No risk, simple strict warning fix.

Comment 7

[Serge Gautherie (:sgautherie)]

Fwiw,

http://mxr.mozilla.org/comm-central/search?string=onUpdateCurrentBrowser&case=on
"Found 5 matching lines in 3 files"

{
/mozilla/browser/base/content/browser.js
    * line 4990 -- onUpdateCurrentBrowser: function XWB_onUpdateCurrentBrowser(aStateFlags, aStatus, aMessage, aTotalProgress) {
}

Bug 583317 ported bug 327604, which added this method to FF.
Should we port it too? (If not somehow already done.)
(I may just be wrong, but I'm wondering if (something like) that might be related to bug 753765...)

{
/mozilla/browser/base/content/tabbrowser.xml
    * line 555 -- this._callProgressListeners("onUpdateCurrentBrowser",
}

Bug 583317 also ported bug 331938, which added this call to FF.
(http://hg.mozilla.org/mozilla-central/rev/087a11f74b53)
(Same idea, though I didn't check what the situation is besides not having this call in SM, afaict.)

Comment 8

[[e-mail]]

We don't actually need to implement that function, it only exists in case extensions are trying to listen for it.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 626361 [details] [diff] [review]
(Av1) Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.mBrowser"| in updateCurrentBrowser()
[Checked in: Comments 9 and 23]

SM 2.12a1:
http://hg.mozilla.org/comm-central/rev/2666d5407c2b

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) browser_popupNotification.js: Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.(value|href)"| in geolocation notification constructor, ... [Checked in: Comments 15 and 24]

(In reply to [e-mail] from comment #1)
> This object is created as part of showGeolocationPrompt, but it is always
> created with both value and href properties. (I assume the href is there
> because there is no strict warning for it...)

Bah, browser_popupNotification.js doesn't use showGeolocationPrompt() but its own basicNotification()...
The href warning is reported once the value warning is fixed ;->

Comment 11

[[e-mail]]

Comment on attachment 626795 [details] [diff] [review]
(Bv1) browser_popupNotification.js: Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.(value|href)"| in geolocation notification constructor, ...
[Checked in: Comments 15 and 24]

Not sure what the deal is with the licence changeover though, I thought that it was going to be all done in one go.

Comment 12

[[e-mail]]

<gerv> might as well leave them.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #11)
> Not sure what the deal is with the licence changeover though, I thought that
> it was going to be all done in one go.

I just resync'ed that part too, while here.


(In reply to [e-mail] from comment #12)
> <gerv> might as well leave them.

I'm not sure what you meant.

Comment 14

[[e-mail]]

Sorry; I'd asked Gerv on IRC whether it was worth changing the licences.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 626795 [details] [diff] [review]
(Bv1) browser_popupNotification.js: Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.(value|href)"| in geolocation notification constructor, ...
[Checked in: Comments 15 and 24]

http://hg.mozilla.org/comm-central/rev/42cb14c91cd6


[Approval Request Comment]
No risk, 95% test-only.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #1)
> (In reply to Serge Gautherie from comment #0)
> > JavaScript strict warning: chrome://global/content/bindings/popup.xml, line
> > 0: reference to undefined property this.popupBoxObject.popupState
> 
> This is even weirder. this.popupBoxObject returns an nsIPopupBoxObject which
> always provides a popupState attribute. So it can never be undefined...

Ftr, this warning goes away if I comment out "window.locationbar.visible = false;".

Comment 17

[[e-mail]]

This is an XPConnect bug.

When showing the popup, we query for the popup state, which flushes layout. This then decides to go a delete a bunch of frames, presumably for the hidden location bar. Then (presumably) the inner input element then tries to clear the command context on all of its controllers.

Now, not all controllers have a command context. So, they call get QueryInterface called on them to find out whether they do. But, there's a controller that doesn't have a QueryInterface method on it. And the way that XPConnect now uses to call QueryInterface causes a strict JS warning when QueryInterface doesn't exist. Worse, the warning gets reported for the original property access, which is actually now being executed...

I suppose you could work around the problem by explicitly flushing layout after hiding the location bar, but I don't know the best way to do that.

Comment 18

[[e-mail]]

Or, as an alternative workaround, you could add QueryInterface to those edit controllers (urlbarBindings.xml and autocomplete.xml) that don't have it.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #17)

> When showing the popup, we query for the popup state, which flushes layout.
> This then decides to go a delete a bunch of frames, presumably for the
> hidden location bar. Then (presumably) the inner input element then tries to
> clear the command context on all of its controllers.

Thanks for the details.

Ftr, all I could "debug" was that the warning was emitted _between_ onShowing and onShown events.
I guess that is when the flush/++ happens ;-)

> And the way that
> XPConnect now uses to call QueryInterface causes a strict JS warning when
> QueryInterface doesn't exist. Worse, the warning gets reported for the

That strict warning sounds like a feature to me.
Are you saying that XPConnect should just silently ignore the missing QueryInterface?

> original property access, which is actually now being executed...

Should I file a bug about getting a proper "missing QueryInterface() on proper (controller) object"?
(Or is there one already?)

> I suppose you could work around the problem by explicitly flushing layout
> after hiding the location bar, but I don't know the best way to do that.

Don't bother.
More interesting will be to find/fix the "broken" controller...


(In reply to [e-mail] from comment #18)
> Or, as an alternative workaround, you could add QueryInterface to those edit
> controllers (urlbarBindings.xml and autocomplete.xml) that don't have it.

Oh, good!
That seems preferable to me.

Again, you say this is a workaround, but
{
http://mxr.mozilla.org/mozilla-central/source/content/xul/document/public/nsIController.idl
9 interface nsIController : nsISupports {
+
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsISupports.idl
26 interface nsISupports {
27   void QueryInterface(in nsIIDRef uuid, 
}
suggests all controllers are required to provide a QueryInterface, arent't they?

NB:
http://mxr.mozilla.org/mozilla-central/find?text=&string=%2Fautocomplete.xml
I wonder why Firefox doesn't seem affected.
Though I noticed its log has the same warning but in a different test: maybe it's somehow reported once only??

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #19)
> http://mxr.mozilla.org/mozilla-central/find?text=&string=%2Fautocomplete.xml
> I wonder why Firefox doesn't seem affected.
> Though I noticed its log has the same warning but in a different test: maybe
> it's somehow reported once only??

Or is it because Toolkit uses an external controller, whereas SeaMonkey uses "self-control"?

Comment 21

[[e-mail]]

(In reply to Serge Gautherie from comment #19)
> (In reply to [e-mail] from comment #17)
> > And the way that
> > XPConnect now uses to call QueryInterface causes a strict JS warning when
> > QueryInterface doesn't exist. Worse, the warning gets reported for the
> That strict warning sounds like a feature to me.
> Are you saying that XPConnect should just silently ignore the missing
> QueryInterface?
Yes; normally you don't need a QueryInterface to pass a JS object to a C++ object, as long as the JS object has all the other methods it's OK.

> More interesting will be to find/fix the "broken" controller...
Well, it's not "broken", XPConnect is.

> I wonder why Firefox doesn't seem affected.
I don't know, its controller(s?) should have the same "bug".

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #19)

> Are you saying that XPConnect should just silently ignore the missing
> QueryInterface?

Bug 758756 Submitted

> I wonder why Firefox doesn't seem affected.
> Though I noticed its log has the same warning but in a different test: maybe
> it's somehow reported once only??

I just notice a "WarnOnceAbout()" function, so I think both apps are affected alike.

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 626361 [details] [diff] [review]
(Av1) Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.mBrowser"| in updateCurrentBrowser()
[Checked in: Comments 9 and 23]

http://hg.mozilla.org/releases/comm-aurora/rev/bf48a2b1e0ce

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 626795 [details] [diff] [review]
(Bv1) browser_popupNotification.js: Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.notification.options.(value|href)"| in geolocation notification constructor, ...
[Checked in: Comments 15 and 24]

http://hg.mozilla.org/releases/comm-aurora/rev/2af9e3e9c30b

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 626361 [details] [diff] [review]
(Av1) Fix 2 |JavaScript Warning: "ReferenceError: reference to undefined property this.mBrowser"| in updateCurrentBrowser()
[Checked in: Comments 9 and 23]

Actually I'm inclined not to pass this through as code churn this late in the game. Ratty brought up good points on my overeagerness to approve some stuff.

Comment 26

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1338445001.1338449007.28783.gz&fulltext=1
OS X 10.6 comm-aurora debug test mochitest-other on 2012/05/30 23:16:41

seamonkey2.11a2: verified, wrt patches A and B.