User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130805 Firefox/24.0 (Nightly/Aurora) Build ID: 20130805004006 Steps to reproduce: During a make SOLO_TEST=content-tabs/test-plugin-crashing.js mozmill-one, I get a warning: JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 352: reference to undefined property aTabType.panelId Actual results: The code does this: aTabType.panel = document.getElementById(aTabType.panelId); Apparently it gets called from several places with an aTabType that does not have the panelId property. Expected results: There should be no warning. And probably the function should not be called with an object that does not have the panelId property.

For all tab types not defining panelId, registerTabType is actually called, according to the stack traces: JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 352: reference to undefined property aTabType.panelId Stacktrace: chrome://messenger/content/tabmail.xml (356) chrome://messenger/content/tabmail.xml (363) chrome://messenger/content/msgMail3PaneWindow.js (386) chrome://messenger/content/messenger.xul (1) null (0) Stacktrace: chrome://messenger/content/tabmail.xml (356) chrome://messenger/content/tabmail.xml (363) chrome://messenger/content/specialTabs.js (491) chrome://messenger/content/msgMail3PaneWindow.js (407) chrome://messenger/content/messenger.xul (1) null (0) Stacktrace: chrome://messenger/content/tabmail.xml (356) chrome://messenger/content/tabmail.xml (363) chrome://messenger/content/specialTabs.js (492) chrome://messenger/content/msgMail3PaneWindow.js (407) chrome://messenger/content/messenger.xul (1) null (0) Stacktrace: chrome://messenger/content/tabmail.xml (356) chrome://messenger/content/tabmail.xml (363) chrome://messenger/content/webSearchTab.js (53) chrome://messenger/content/msgMail3PaneWindow.js (408) chrome://messenger/content/messenger.xul (1) null (0) Stacktrace: chrome://messenger/content/tabmail.xml (356) chrome://messenger/content/tabmail.xml (363) chrome://messenger/content/msgMail3PaneWindow.js (409) chrome://messenger/content/messenger.xul (1) null (0)

The question is whether panelId is mandatory (as the patch shows, it isn't in several cases, which then would have to be fixed), or may be omitted (in which case it would be sufficient to add "if (aTabType.panelId)" before using the property). Documentation at the beginning of tabmail.xml answers this question: - The tab type definition should include the following attributes: - * name: The name of the tab-type, mainly to aid in debugging. - * panelId or perTabPanel: If using a single tab panel, the id of the - panel must be provided in panelId. If using one tab panel per tab, - perTabPanel should be the XUL element name that should be created for - each tab. If a reason arises, in the future we might support - this being a helper function to create and return the element.

As per the documentation (and actual implementation) in tabmail.xml, either aTabType.panelId or aTabType.perTabPanel is sufficient. Therefore, we should not rely on this property being set. While looking at the different tab types, I also found one inaccurate comment about the place where that type is defined and fixed it; I also added another such comment.

This error is also shown when starting Daily normally. I do not think the problem should be fixed in the way you do. If the comment in tabmail.xml is right, you should check if either panelId or perTabPanel is set (and fetch .panelId only if perTabPanel is not set). Still, there are some places where registerTabType() is called with an object where none of those is set and those should be investigated. Also it seems Seamonkey has similar code in its tabmail.xml so may be affected too.

Comment on attachment 788548 [details] [diff] [review] 903636-fix.patch I'm not a valid Thunderbird reviewer anymore. I haven't looked at the diff in context, but I assume this is the static registration logic. Some type of comment is likely appropriate at the call-site along the following lines: // If this tab type uses a single panel (potentially used multiple times), // then it must be an overlay that is already present. Resolve it now. and then an else case that resolves to null would be desired. Something like so. // Otherwise, we will resolve the panel when the tab is instantiated aTabType.panel = null;

(In reply to :aceman from comment #5) > Still, there are some places where registerTabType() is called with an > object where none of those is set and those should be investigated. I did a full text search for registerTabType and found the following calls: * In messenger-overlay-sidebar.js (Lightning). Passes calendarTabType, which has a panelId. * In msgMail3PaneWindow.js: - mailTabType: Defined in mailTabs.js, has a panelId - glodaFacetTabType: Defined in glodaFacetTab.js, has perTabPanel: "vbox" - accountProvisionerTabType: Defined in accountProvisionerTab.js, apparently has neither panelId nor perTabPanel ===> To be investigated ... * In specialTabs.js: - contentTabType: Has perTabPanel: "vbox" - chromeTabType: Has perTabPanel: "vbox" * In webSearchTabs.js: "this", has perTabPanel: "vbox" * In chat-messenger-overlay.js: chatTabType, has a panelId * In suite/mailnews/msgMail3PaneWindow.js: gMailNewsTabsType, defined in tabmail.js, has panelId: "mailContent" So unless I am mistaken, one out of nine occurences to be investigated.

(In reply to :aceman from comment #5) > This error is also shown when starting Daily normally. > > I do not think the problem should be fixed in the way you do. > If the comment in tabmail.xml is right, you should check if either panelId > or perTabPanel is set (and fetch .panelId only if perTabPanel is not set). And what would you do if either both or none of them are set? Either case is an invalid parameter, IMO. But we also do not check so strictly for invalid parameters elsewhere. What I had in mind when proposing the fix were _valid_ parameters, for which the warning was also triggered.

(In reply to Jens Müller (:tessarakt) from comment #8) > And what would you do if either both or none of them are set? Either case is > an invalid parameter, IMO. But we also do not check so strictly for invalid > parameters elsewhere. We could show an error to the Error console. Because with the patch you now hide the error that accountProvisionerTabType has no panelId or perTabPanel. > What I had in mind when proposing the fix were _valid_ parameters, for which > the warning was also triggered. We don't yet know which of the passed in objects caused this warning so we do not know if its parameters were valid. But if you check only one of panelId or perTabPanel is set then the change in the patch can stay as is.

(In reply to :aceman from comment #9) > (In reply to Jens Müller (:tessarakt) from comment #8) > > And what would you do if either both or none of them are set? Either case is > > an invalid parameter, IMO. But we also do not check so strictly for invalid > > parameters elsewhere. > > We could show an error to the Error console. > Because with the patch you now hide the error that accountProvisionerTabType > has no panelId or perTabPanel. Is Components.utils.reportError sufficient? > > What I had in mind when proposing the fix were _valid_ parameters, for which > > the warning was also triggered. > > We don't yet know which of the passed in objects caused this warning Indeed. I should have checked (and I will do that). > so we > do not know if its parameters were valid. But if you check only one of > panelId or perTabPanel is set then the change in the patch can stay as is. No, I don't check that yet.

(In reply to Jens Müller (:tessarakt) from comment #10) > Is Components.utils.reportError sufficient? Yes.

It would be nice if this warning can be eliminated somehow. From the |make mozmill| test run of DEBUG BUILD of comm-central TB (the source was refreshed the day before), I get the following Javascript strict warning and this warning showed up most frequently. The number preceding the error message line is the frequency of appearances. ======================================== JavaScript strict warning jquery.js and jquery-ui.js are ignored. ======================================== 55 JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 352: reference to undefined property aTabType.panelId 55 JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 50: reference to undefined property this.treeBoxObject.columns 55 JavaScript strict warning: chrome://global/content/bindings/findbar.xml, line 262: reference to undefined property this._browser.finder 35 JavaScript error: chrome://global/content/bindings/findbar.xml, line 262: this._browser.finder is undefined 12 JavaScript strict warning: chrome://messenger/content/newmailaccount/jquery.tmpl.js, line 123: reference to undefined property def[1] 11 JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 53: reference to undefined property this.treeBoxObject.view 3 JavaScript strict warning: chrome://messenger/content/nsContextMenu.js, line 102: reference to undefined property this.onEditableArea 1 JavaScript strict warning: resource:///modules/dbViewWrapper.js, line 1046: reference to undefined property this._underlyingFolders[0] TIA

(In reply to Jens Müller (:tessarakt) from comment #7) > - accountProvisionerTabType: Defined in accountProvisionerTab.js, > apparently has neither panelId nor perTabPanel > ===> To be investigated ... > * In specialTabs.js: > - contentTabType: Has perTabPanel: "vbox" > > So unless I am mistaken, one out of nine occurences to be investigated. The comment in accountProvisionerTab.js sais that accountProvisionerTabType "subclasses" specialTabs.contentTabType so it inherits perTabPanel="vbox" (tested). So it does not need to be investigated. So if you do not want to add the test with report to error console if neither panelId nor perTabPanel is defined, I am OK with the current patch. Can you propose it to review? I see Seamonkey also has a similar unconditional assignment in its tabmail.xml so I wonder if it has the same problem.

What is the status of this patch? Does it require review? This warning also shows up on running TB, no need to use mozmill to reproduce.

Comment on attachment 788548 [details] [diff] [review] 903636-fix.patch Review of attachment 788548 [details] [diff] [review]: ----------------------------------------------------------------- Can you review and/or update this patch? From the comments above, you seem to have thought it through already. Thanks!

Comment on attachment 788548 [details] [diff] [review] 903636-fix.patch Review of attachment 788548 [details] [diff] [review]: ----------------------------------------------------------------- It seems to me tessarakt needs to update the patch according to the last comments. Or is he away?

(In reply to :aceman from comment #16) > It seems to me tessarakt needs to update the patch according to the last > comments. Or is he away? A year ago, you said: > The comment in accountProvisionerTab.js sais that accountProvisionerTabType > "subclasses" specialTabs.contentTabType so it inherits perTabPanel="vbox" > (tested). So it does not need to be investigated. > > So if you do not want to add the test with report to error console if > neither panelId nor perTabPanel is defined, I am OK with the current patch. > Can you propose it to review? This sounds to me like you thought then the patch was OK? If you think tesseract still has to change something, needinfo him, but let's get this done ;)

OK, I have checked the history here and the code and will look at the resolution.

(In reply to :aceman from comment #18) > OK, I have checked the history here and the code and will look at the > resolution. Great :-) I just fixed one of the other noisy ones on startup (bug 1177709), so maybe we'll get less spam in the console soon.

Comment on attachment 788548 [details] [diff] [review] 903636-fix.patch Review of attachment 788548 [details] [diff] [review]: ----------------------------------------------------------------- So I tried to also add the reporting of tabs that do not have either panelID or perTabPanel set and I couldn't find any. When perTabPanel is set, the tab.panel is initialized in other function so it may be fine we no not do it here in registerTabType. So I think this is done. Please refresh the patch to have proper format and fix the bitrot in msgMail3PaneWindow.js where it doesn't apply cleanly anymore.

Comment on attachment 8670944 [details] [diff] [review] Avoid reference to undefined property aTabType.panelId in tabmail.xml Review of attachment 8670944 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for refreshing the patch. Please keep tessaract as the author of the patch to give him credit for investigating the issue :) ::: mail/base/content/tabmail.xml @@ +353,5 @@ > + aTabType.panel = document.getElementById(aTabType.panelId); > + else if (!aTabType.perTabPanel) { > + throw("Trying to register a tab type with neither panelId " + > + "nor perTabPanel attributes.") > + } I said this does not need to be done, but if you added it I'm fine with it.

Bah, that was bzexport changing the ASSIGNED automatically.

https://hg.mozilla.org/comm-central/rev/5c600f5451df7ca29e49a2e79934126008a448aa Bug 903636 - Avoid reference to undefined property aTabType.panelId in tabmail.xml. r=aceman a=aleth