Browser 'type' attribute behaviour change following bug 1322414

VERIFIED FIXED in Thunderbird 53.0

Status

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Paenglab, Assigned: aleth)

Tracking

({regression})

Trunk
Thunderbird 53.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Today build opens without any entry in folderPane.

I get this errors:

Fri Dec 16 2016 12:09:30
Error: TypeError: this._browser.messageManager is null
Source file: chrome://global/content/bindings/findbar.xml
Line: 296

Fri Dec 16 2016 12:09:31
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.sessionHistory]
Source file: chrome://messenger/content/specialTabs.js
Line: 573

Fri Dec 16 2016 12:09:36
Error: TypeError: gFolderTreeView._treeElement is undefined
Source file: chrome://messenger/content/folderPane.js
Line: 269
Could bug 1320391 be the culprit?
I'm using the Daily of 2016-12-15 (yesterday) and it works fine. Since then, nothing landed on C-C, so the bustage comes from M-C.

If TB is unusable, we will get all tests completely red in a minute when the Daily compile of today finishes .

I can look into it in the afternoon.
Backout of bug 1320391 doesn't help. Building on m-c rev fe17931bfd5f let's TB usable.
Thanks, so the regression range is
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fe17931bfd5f&tochange=63b447888a64

I'm building now to see what's happening. Back in a few hours.
I'm almost sure bug 1322609 is the culprit.

It changes type="content-primary" to type="content" primary="true"
Looking at bug 1322609, they've changed this:
-              oldBrowser.setAttribute("type", "content-targetable");
+              oldBrowser.setAttribute("type", "content");
https://hg.mozilla.org/mozilla-central/rev/4616e2bb5fc4

If I do this in all occurrences in TB, TB starts again. There are still JS errors and we still need to look at bug 1322414 where this was changed:
https://hg.mozilla.org/mozilla-central/rev/e3c689dbcd92
-              oldBrowser.setAttribute("type", "content");
+              oldBrowser.removeAttribute("primary");
               oldBrowser.docShellIsActive = false;
-              newBrowser.setAttribute("type", "content-primary");
+              newBrowser.setAttribute("primary", "true");

I'll attach my patch and try to catch Gijs on IRC.
With this patch, TB at least starts and shows the folder pane.
Comment on attachment 8819293 [details] [diff] [review]
Part 1: Port bug 1322609: content-targetable -> content

Looks like this is right. We need part 2 that does something with content-primary. Coming up.
Attachment #8819293 - Attachment description: WIP (v1). → Part 1: Port bug 1322609: content-targetable -> content
Posted patch 1323968.patchSplinter Review
Full patch with both parts.

I only still get the findbar.xml error.
Attachment #8819297 - Flags: review?(jorgk)
Comment on attachment 8819293 [details] [diff] [review]
Part 1: Port bug 1322609: content-targetable -> content

Richard beat me to it. His patch looks good and includes my changes.
Attachment #8819293 - Attachment is obsolete: true
Pushed here:
https://hg.mozilla.org/comm-central/rev/42aa2df379ce3ffa520f01c2799a9e4283c45327
(I fixed an over-long line in msgPrintEngine.xul).

Thanks to Richard for investigating a providing a patch.

I'm marking this "leave-open" since there still seem to be a few issues left here:
1) What is "content-conversation" all about?
   http://searchfox.org/comm-central/search?q=content-conversation&case=true&regexp=false&path=
   That's mostly used in chat or IM
2) There are a few "content-targetable" left.
   http://searchfox.org/comm-central/search?q=content-targetable&case=true&regexp=false&path=
   We missed two comments and there are two occurrences in suite/
3) We still see the chrome://global/content/bindings/findbar.xml#296 error
   (which may be unrelated).

NI: Aleth for 1) and Ratty for 2)
Flags: needinfo?(philip.chee)
Flags: needinfo?(aleth)
Keywords: leave-open
Comment on attachment 8819297 [details] [diff] [review]
1323968.patch

Thanks again.
Attachment #8819297 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+1) from comment #11)
> 1) What is "content-conversation" all about?
>   
> http://searchfox.org/comm-central/search?q=content-
> conversation&case=true&regexp=false&path=
>    That's mostly used in chat or IM

That refers to the chat browser XBL. Unless there are bug reports, I don't think it will be affected by the change here.
Flags: needinfo?(aleth)
Aleth, please see answer in bug 1322414 comment #25 (quote):

It wants to be type="content". If it was never set to type="content-primary" before, I suspect that's all you need to do.
Flags: needinfo?(aleth)
I still don't believe content-conversation is affected at all. We define that ourselves in convbrowser.xml.
Flags: needinfo?(aleth)
Duplicate of this bug: 1324056
Blocks: 1324121
See Also: → 1324121
No longer blocks: 1324121
While with the landing in comment #11 TB seems to run, we're still seeing a lot of Mozmill failures here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8afa0892704bf83bc0168407fa71abf6234718c9
(In reply to aleth [:aleth] from comment #16)
> I still don't believe content-conversation is affected at all. We define
> that ourselves in convbrowser.xml.

Looks like that was too optimistic :-(
(In reply to Jorg K (GMT+1) from comment #15)
> Aleth, please see answer in bug 1322414 comment #25 (quote):
> 
> It wants to be type="content". If it was never set to type="content-primary"
> before, I suspect that's all you need to do.

Looks like what was meant by that comment is that no custom values are supported any more.
See also bug 1322414 comment 18. This patch also fixes up the broken Instantbird.
Attachment #8819472 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Ratty already has his own bug now, so clearing NI.

The test failures here:
https://archive.mozilla.org/pub/thunderbird/try-builds/aleth@instantbird.org-db7e6b2b78ffe19d529c135fa81ad2b5feb9f4ff/try-comm-central-linux64/try-comm-central_ubuntu64_vm_test-mozmill-bm114-tests1-linux64-build4.txt.gz
have
  window.content is null
51 times. I wonder where this is coming from.
Flags: needinfo?(philip.chee)
Assignee: aleth → richard.marti
Summary: TB (2016-12-16) unusable. No entries in folderPane → Browser 'type' attribute behaviour change following bug 1322414
(In reply to Jorg K (GMT+1) from comment #22)
> have
>   window.content is null
> 51 times. I wonder where this is coming from.

Could be a regression or a race condition? From https://developer.mozilla.org/en-US/docs/Web/API/Window/content it seems window.content shouldn't be null.
Sorry, I quoted this incompletely. This is how they really look:
https://archive.mozilla.org/pub/thunderbird/try-builds/aleth@instantbird.org-db7e6b2b78ffe19d529c135fa81ad2b5feb9f4ff/try-comm-central-linux64/try-comm-central_ubuntu64_vm_test-mozmill-bm114-tests1-linux64-build4.txt.gz

EXCEPTION: aController.window.content is null
EXCEPTION: troller.window.content is null - funny, the "troller", was that also meant to be "aController"?

The Mozmill failures are not related as the following analysis shows:

Being at M-C rev 63b447888a64 I've just run
mozmake SOLO_TEST=content-policy/test-js-content-policy.js mozmill-one
locally and it also fails with
EXCEPTION: noscript display should be 'inline'; display=none
so the failure we fixed in bug 1316256.

Applying only
https://hg.mozilla.org/mozilla-central/rev/7ba8dff4a242
locally
makes the error go away.

So these Mozmill failures are related to something that landed on M-C after M-C rev 63b447888a64 were I'm currently at. Bug 1316256 landed after M-C rev 63b447888a64 and applying it *only* fixes test-js-content-policy.js instead of breaking it further.

Therefore the Mozmill failures are not related to this bug here but came in between 63b447888a64 and 5a536a16e337:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=63b447888a64&tochange=5a536a16e337

I've raised bug 1324185 for that.

The findbar.xml problem (comment #11 item 3)) is still to be looked at.

Aceman, when you have some time, would you be able to look at these two issues, bug 1324185 and the findbar.xml issue?
Flags: needinfo?(acelists)
Comment on attachment 8819472 [details] [diff] [review]
Browser binding type must be set to 'content' following bug 1322414

This looks right and works according to Richard's test.
Attachment #8819472 - Flags: review+
I've rebuild on C-C and M-C tip and can't reproduce the Mozmill failures locally, see bug 1324185 comment #1.

Still showing up is
JavaScript error: chrome://global/content/bindings/findbar.xml, line 296: TypeError: this._browser.messageManager is null

From time to time I also get:
autosyncActivities      ERROR   onDownloadCompleted: TypeError: this._syncInfoPerFolder.get(...) is undefined
JavaScript error: resource:///modules/activity/autosync.js, line 309: TypeError: this._syncInfoPerFolder.get(...) is undefined

Maybe unrelated.
(In reply to Jorg K (GMT+1) from comment #26)
> Still showing up is
> JavaScript error: chrome://global/content/bindings/findbar.xml, line 296:
> TypeError: this._browser.messageManager is null

This one is caused by the findbar referencing a browser with id=conv-log-browser , probably at https://dxr.mozilla.org/comm-central/rev/5a163ed948ceb5e930d754a6491599d12471d94a/mail/components/im/content/chat-messenger-overlay.xul#247. Aleth does it need the "type" treatment too? Will you include it in your patch?
Flags: needinfo?(acelists) → needinfo?(aleth)
(In reply to Jorg K (GMT+1) from comment #26)
> From time to time I also get:
> autosyncActivities      ERROR   onDownloadCompleted: TypeError:
> this._syncInfoPerFolder.get(...) is undefined
> JavaScript error: resource:///modules/activity/autosync.js, line 309:
> TypeError: this._syncInfoPerFolder.get(...) is undefined

Yes, I think this is unrelated, looks the same as bug 534602, which you may reopen (just that [folder.URI] got changed to .get(folder.URI) since the report, but the cause why the item does not exist in the object/map was never fixed).
Duplicate of this bug: 1324199
(In reply to :aceman from comment #27)
> (In reply to Jorg K (GMT+1) from comment #26)
> > Still showing up is
> > JavaScript error: chrome://global/content/bindings/findbar.xml, line 296:
> > TypeError: this._browser.messageManager is null
> 
> This one is caused by the findbar referencing a browser with
> id=conv-log-browser , probably at
> https://dxr.mozilla.org/comm-central/rev/
> 5a163ed948ceb5e930d754a6491599d12471d94a/mail/components/im/content/chat-
> messenger-overlay.xul#247. Aleth does it need the "type" treatment too? Will
> you include it in your patch?

It's already in the patch, thanks.
Flags: needinfo?(aleth)
r+ by florian over IRC, with xbltype renamed to browser-type (I didn't use content-type because it already gives a ton of hits in c-c).
Attachment #8819472 - Attachment is obsolete: true
Attachment #8819472 - Flags: review?(clokep)
Assignee: richard.marti → aleth
Assignee: aleth → richard.marti
https://hg.mozilla.org/comm-central/rev/7d0c00119801e5601b42d0ab4417a22f1892b7f1
Bug 1323968 - Browser binding type must be set to 'content' following bug 1322414. r=florian,jorgk CLOSED TREE
Depends on: 1324185
Off-hand this patch doesn't look like it ever sets primary=true, which might explain the mozmill failures in the other bug if they don't disappear after this fix.
I suspect this should take care of the mozmill failures - Paenglab's patch forgot that in some places the primary attribute will already be set, so setting type=content isn't enough.
Assignee: richard.marti → aleth
(In reply to :Gijs Kruitbosch from comment #33)
> Off-hand this patch doesn't look like it ever sets primary=true, which might
> explain the mozmill failures in the other bug if they don't disappear after
> this fix.

If this was referring to the patch landed in comment 32, I don't think we need to set primary=true there as type != "content-primary" wasn't previously ever interpreted as primary, if I understand this correctly: https://hg.mozilla.org/mozilla-central/rev/e3c689dbcd92#l21.41
Comment on attachment 8819614 [details] [diff] [review]
Followup to also remove the primary attribute where required

Yes, of course, now it becomes clear.

r=jorgk, but then we already know what my review is worth :-(
Attachment #8819614 - Flags: review+
https://hg.mozilla.org/comm-central/rev/ccacb490f415c1cdc6d5d2aba79621cae637a3d2
Bug 1323968 - Followup to also remove the primary attribute where required. r=jorgk CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
2016-12-16 Daily build(32bits) was updatred to 2016-12-17 build by software update on Win 10, and folder pane came back and worked.
-> verified.

Thanks for hard works and quick fixing.
Status: RESOLVED → VERIFIED
Blocks: TB52found
You need to log in before you can comment on or make changes to this bug.