1323968 - Browser 'type' attribute behaviour change following bug 1322414

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Richard Marti (:Paenglab)]

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

Comment 1

[Richard Marti (:Paenglab)]

Could bug 1320391 be the culprit?

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[Richard Marti (:Paenglab)]

Backout of bug 1320391 doesn't help. Building on m-c rev fe17931bfd5f let's TB usable.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Richard Marti (:Paenglab)]

I'm almost sure bug 1322609 is the culprit.

It changes type="content-primary" to type="content" primary="true"

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Part 1: Port bug 1322609: content-targetable -> content

With this patch, TB at least starts and shows the folder pane.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: 1323968.patch

Full patch with both parts.

I only still get the findbar.xml error.

Comment 10

[Jorg K (CEST = GMT+2)]

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.

Comment 11

[Jorg K (CEST = GMT+2)]

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)

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8819297 [details] [diff] [review]
1323968.patch

Thanks again.

Comment 13

[aleth [:aleth]]

(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.

Comment 14

[Jorg K (CEST = GMT+2)]

Also see bug 1322414 comment #24.

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[aleth [:aleth]]

I still don't believe content-conversation is affected at all. We define that ourselves in convbrowser.xml.

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[aleth [:aleth]]

(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 :-(

Comment 20

[aleth [:aleth]]

(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.

Comment 21

[aleth [:aleth]]

Attachment: Browser binding type must be set to 'content' following bug 1322414

See also bug 1322414 comment 18. This patch also fixes up the broken Instantbird.

Comment 22

[Jorg K (CEST = GMT+2)]

Ratty already has his own bug now, so clearing NI.

The test failures here:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-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.

Comment 23

[aleth [:aleth]]

(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.

Comment 24

[Jorg K (CEST = GMT+2)]

Sorry, I quoted this incompletely. This is how they really look:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-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?

Comment 25

[Jorg K (CEST = GMT+2)]

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.

Comment 26

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[:aceman]

(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?

Comment 28

[:aceman]

(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).

Comment 30

[aleth [:aleth]]

(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.

Comment 31

[aleth [:aleth]]

Attachment: Browser binding type must be set to 'content' following bug 1322414

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).

Comment 32

[aleth [:aleth]]

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

Comment 33

[:Gijs (he/him)]

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.

Comment 34

[aleth [:aleth]]

Attachment: Followup to also remove the primary attribute where required

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.

Comment 35

[aleth [: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 36

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 37

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ccacb490f415c1cdc6d5d2aba79621cae637a3d2
Bug 1323968 - Followup to also remove the primary attribute where required. r=jorgk CLOSED TREE

Comment 38

[WADA:World Anti-bad-Duping Agency]

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.