179787 - JavaScript strict "Warning: reference to undefined property this.mPanelContainer" in <tabbrowser.xml>

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Henrik Gemal]

I'm started to seem a bunch of these inthe console:
Warning: reference to undefined property this.mPanelContainer

They are almost flloding my console

lxr reports this as:
/xpfe/global/resources/content/bindings/tabbrowser.xml
/toolkit/content/widgets/tabbrowser.xml

Comment 1

[Wesha]

Steps to reproduce please?

Comment 2

[Boris Zbarsky [:bzbarsky]]

tabbrowser

Comment 3

[Brant Gurganus]

Build 2002122608
First, these only appear when Strict JavaScript Warning are on.
Second, these only appear when a new Navigator window is created in addition to
the initial window created when Mozilla is launched.

Therefore, steps to reproduce:
1. Turn Strict JavaScript warnings on.  Make sure new Navigator windows are blank.
2. Exit Mozilla
3. Launch Mozilla Navigator.
4. Open the JavaScript Console.  You will see that this warning is not yet present.
5. Open a second Navigator window.
6. View the JavaScript Console.  You will see a large amount of warnings
regarding the undefined property this.mPanelContainer.

For solving the issue, you can view the article about strict JavaScript warnings
at <http://www.javascriptkit.com/javatutors/serror.shtml>.

Comment 4

[Greg K.]

This also happens on every tab switch using FizzillaMach/2003040103. Setting
Hardware=All.

Comment 5

[Greg K.]

So either all those references to this.mPanelContainer need to use the fail-safe
detection method referred to in the article, or this.mPanelContainer need to be
explicitly defined somewhere.

I'm going to suggest this be fixed for 1.4 final by setting blocking1.4?. This
clutters up the JS Console with lots of warnings that reduce its' usability for
other purposes, and I imagine it's probably easy to fix.

Comment 6

[Greg K.]

Seemingly introduced between 2002110808 and 2002112208 FizzillaCFM builds. If we
assume it was introduced prior to the date/time of this bug being filed, that
narrows it down to approximately four days, represented by these check-ins:
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F08%2F2002+08%3A46%3A00&maxdate=11%2F12%2F2002+12%3A50%3A00&cvsroot=%2Fcvsroot>.

Could bug 168807 or bug 173703 be involved?

Comment 7

[Asa Dotzler [:asa]]

If we see a fix for this we'll consider it for 1.4 but not going to block 1.4
for noise in the JS console (which isn't there for default profiles and most
milestone users, right?)

Comment 8

[David Mårtensson]

I get the same problem opening a popup window from javascript width

window.open(...


Casues a lot of searching when developing pages since I use the javascript
consol alot to debug scripts.

Comment 9

[Karsten Düsterloh]

AFAICT from some very excessive dump session in
mozilla\kddev\dist\bin\chrome\toolkit\content\global\bindings\tabbrowser.xml,

      <field name="mStrip">
        this.mTabBox.firstChild
      </field>

is to blame: it cause the 

      <handler event="select" action="if (event.originalTarget ==
this.mPanelContainer) this.updateCurrentBrowser();"/>

to be called, but mPanelContainer hasn't been initialized yet (it's initialized
two fields after mStrip)...

A simple patch might be checking the validity of mPanelContainer first:

      <handler event="select">
        <![CDATA[
          if (("mPanelContainer" in this) && (event.originalTarget ==
this.mPanelContainer))
            this.updateCurrentBrowser();
        ]]>
      </handler>

(Moving the mPanelContainer field before mStrip doesn't help, because
updateCurrentBrowser would be still unknown.)

Comment 10

[Karsten Düsterloh]

Path should be \mozilla\xpfe\global\resources\content\bindings\tabbrowser.xml,
of course.

Comment 11

[Karsten Düsterloh]

Attachment: patch as per comment 9

Correction to comment 9:
The "select" event handler invocation is caused not by |mStrip|, but by
      <field name="mTabContainer">
	this.mStrip.childNodes[2]
      </field>
The event handler is called twice, each time causing 9 error messages (this is
bug 104549, I guess).

The correction above has no effect on the suggested patch.

Comment 12

[jag (Peter Annema)]

Erh, but why does that cause that handler to be called?

Comment 13

[Karsten Düsterloh]

> why does that cause that handler to be called?

Good question, that can't answer (yet).

The changes to the select handler are actually just a workaround - and as such
something I'm not quite happy with, since the real problem is still hidden.

I can try to dig further into this, but maybe some 'wizard' can give me hint...

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 127077 [details] [diff] [review]
patch as per comment 9

I'm not sure you can do any better than this, as the updateCurrentBrowser()
method calls on other properties of its "this" object a lot.  To properly fix
it might involve rearranging a good part of the file; I think this solution is
adequate.

>+      <handler event="select"/>

Um, if you check that in you'll get an XML parse error, due to the closing
slash inside the tag.  Otherwise, it looks good.

Comment 15

[Karsten Düsterloh]

Attachment: fix per comment 14

>+	<handler event="select"/>

I had this error in my tree, but wasn't aware that it was in the patch also (I
thought I had just screwed up my local file later).
Sorry; corrected patch attached.

Comment 16

[HJ]

Lets speedup the event handler a bit, no more useless deck/tabpanel and other
events.

-      <handler event="select" action="if (event.originalTarget ==
this.mPanelContainer) this.updateCurrentBrowser();"/>
+      <handler event="select" action="if (event.originalTarget.nodeName ==
'tabs') this.updateCurrentBrowser();"/>

Note: I didn't check Menu/View/Site Navigation Navbar, any takers?

Comment 17

[HJ]

CC'ing Neil.

Neil, I guess you still remember bug 179787, right?

Comment 18

[HJ]

Woops, make that bug 173703

Comment 19

[[e-mail]]

Yes and I also remember that my original patch used
<xul:tabpanels flex="1" class="plain" onselect="event.preventBubble();">
so thet only the <tabs> select events hit the event handler, which simply calls
updateCurrentBrowser() in my tree.

Comment 20

[HJ]

|event.originalTarget.nodeName == 'tabs'| is basically the same thing, with one
exeption, it works without the strict warning ;)

Comment 21

[[e-mail]]

Ah, but you're not blocking tabs in content that way...

Comment 22

[timeless]

This is almost certainly my fault (although i blame our xbl implementation for
not having a strict processing order). I'm fairly certain someone can take the
following and figure out what's happening.

<?xml version="1.0"?><!DOCTYPE bindings><bindings id="x"
xmlns="http://www.mozilla.org/xbl">
<binding id="y"><implementation>
<field name="z">1</field>
<constructor><![CDATA[ this.z = this.z; ]]></constructor>
</implementation></binding></bindings>

bz mentioned bug 63370.

Comment 23

[Boris Zbarsky [:bzbarsky]]

To answer comment 12, here is the sequence of events:

1)  The mTabContainer field is compiled and evaluated
2)  This causes this.mStrip.childNodes[2] to be accessed (per comment 11).  That
    node is the xul:tabs in the tabbox.
3)  Accessing a node like this causes a XPConnect wrapper to be created for it.
    nsElementSH::PostCreate ensures that any relevant XBL bindings are attached
    to the node (in this case the
    "chrome://global/content/bindings/tabbox.xml#tabs" binding).
4)  The constructor of the #tabs binding sets selectedIndex to 0 (see
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbox.xml#194)
    on the tabs.
5)  The selectedIndex setter for #tabs synchronously dispatches a onselect event
    on the <xul:tabs> element (see
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbox.xml#251)
6)  This event bubbles up to the <tabbrowser>, triggering its onselect handler. 
    At this point, note, mPanelContainer is NOT initialized yet.  Hence the
    warnings.

This is not an XBL bug, as far as I can tell....

Comment 24

[[e-mail]]

Attachment: Proposed patch

We prefer the tabs constructor to select tabs, because some themes depend on
the additional attributes being set on adjacent tabs. However in this case
there are no adjacent tabs so this should be a reasonably safe fix.

Comment 25

[HJ]

Comment on attachment 136860 [details] [diff] [review]
Proposed patch

This works for me, r+ HJ

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136860 [details] [diff] [review]
Proposed patch

So why does this work?	May I suggest adding a comment explaining why that
attribute is necessary?

Comment 27

[HJ]

Because it skips step 5.

Comment 28

[HJ]

See also:
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbox.xml#226

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136860 [details] [diff] [review]
Proposed patch

I see.	sr=bzbarsky with a nice comment explaining that added.

Comment 30

[[e-mail]]

Sorry, you can't add comments to XBL <content>, because they'd get cloned and
mess up the child node counts.

Comment 31

[Boris Zbarsky [:bzbarsky]]

<sigh>... That's ridiculous.  No wonder XBL bindings are totally undocumented
and unreadable...  We should change the XBL sink to ignore comment nodes inside
<content>, maybe?

Comment 32

[Alex Vincent [:WeirdAl]]

bz: bug 112842 was filed for comments in bindings.  No one's touched it since 
it was filed.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Very well.  Marking dependency so we can remember to come back here and deal
with it once that bug is fixed...

Comment 34

[[e-mail]]

We don't actually need this urgently, right?

Comment 35

[HJ]

Neil, did you receive my e-mail about the MultiZilla crasher?

note: I keep crashing on program exits when I add selected="true" but only when
MultiZilla is installed so I guess something strange is going on.

Comment 36

[[e-mail]]

HJ: yeah, but I couldn't install MultiZilla (mozdev wasn't responding); in the
meantime I tried to reply to you too - I suggested changing tabbox.xml's
_selectedPanel default to this.firstChild instead of null

Comment 37

[[e-mail]]

Well I couldn't reproduce the crash... now all I have to figure out is how to
uninstall Multizilla...

Comment 38

[[e-mail]]

Fix checked in.

Comment 39

[HJ]

Just for the record. Is the proposed patch checked in now, because that's the
one I keep crashing with (:

Comment 40

[Boris Zbarsky [:bzbarsky]]

Please file a bug on documenting this change; make it depend on the "comments in
XBL should be dropped" bug.

Comment 41

[HJ]

Neil, your patch introduced bug 229105

Comment 42

[Serge Gautherie (:sgautherie)]

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

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: port to toolkit

includes bug 229105's fix

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: port to toolkit, merged to trunk (checked in)

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167952 [details] [diff] [review]
port to toolkit, merged to trunk (checked in)

toookit port checked in.