1373565 - Windows jump and tasks lists rollup for c-c applications

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Frank-Rainer Grahl (:frg)]

This might (or might not) be responsible for some startup crashes lately (see bug 1372522).

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1373565-CommitListBuild.patch

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8878381 [details] [diff] [review]
1373565-CommitListBuild.patch

removed too much for suite.

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1373565-CommitListBuild-V2.patch

Only suite tested but looks straight forward.

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1373565-CommitListBuild-part2.patch

Not the only bug missing. At least Bug 692295 also needs to be taken care of but only for 56/2.53.

Bug 692295 - nsWindowsShellService's ShortcutMaintenance can be removed ~ 6/12 (Gecko 56)
Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm
Bug 1356569 - Remove appendElement's last parameter when it is false
Bug 1356569 - Remove addObserver's last parameter when it is false

Let me know if you prefer a different bug for this one. Otherwise I will change the title here.

I switched Components.utils to Cu. Already declared Cc and Ci and mixing doesn't make much sense imho.

Comment 5

[Jorg K (CEST = GMT+2)]

For reference:
Bug  692295 - https://hg.mozilla.org/mozilla-central/rev/1c26e57c1899
Bug 1356569 - https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3

Richard, can you please do a try build on Windows and let us know whether that fixes your crashes. Thank you in advance.

> Let me know if you prefer a different bug for this one. Otherwise I will change the title here.
No, it's all related, so let's deal with it here. Call it "roll up" or so ;-)

Comment 6

[Richard Marti (:Paenglab)]

I have no access to try until this evening. If you could start a try now, then I can test it when I'm home.

Comment 7

[Frank-Rainer Grahl (:frg)]

If it is fixed then it is/was probably caused by bug 1356569.

I already experienced some crashes in 2.52a1:

https://crash-stats.mozilla.com/report/index/90f02fac-3e38-457f-b60c-3e20d0170613

Seems to be stable right now. 

The updated 56a1 Daily crashed fro me once immediately during startup.

https://crash-stats.mozilla.com/report/index/1867f665-4e5f-414c-ba12-999180170615

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #6)
> I have no access to try until this evening. If you could start a try now,
> then I can test it when I'm home.
Done: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7f489522b7c83224799931e44e717023d6f3971b
I'm annoyed be those crashes, too, but after a crash, it usually starts again.

Comment 9

[Frank-Rainer Grahl (:frg)]

Just built a local x64 Daily and it didn't crash after install and during startup. Letting it sit  awile idling away and see what happens.

Comment 10

[Richard Marti (:Paenglab)]

Yes, the try build works without any problems. 
Thanks a lot Frank-Rainer to find the solution. :-)

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878414 [details] [diff] [review]
1373565-CommitListBuild-V2.patch

Yes, looks good.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878415 [details] [diff] [review]
1373565-CommitListBuild-part2.patch

I've compared this visually to the M-C changesets and it looks correct.

Do you want to wait for Ian? Those crashes are just a little annoying, so I'd like to land this sooner than later.

Comment 13

[Ian Neal]

Comment on attachment 8878415 [details] [diff] [review]
1373565-CommitListBuild-part2.patch

>+++ b/suite/modules/WindowsJumpLists.jsm

>-XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
>-  Components.utils.import("resource://gre/modules/NetUtil.jsm");
>-  return NetUtil;
>-});

>@@ -501,22 +478,22 @@ var WinTaskbarJumpList =
>       return;
>     var URIsToRemove = [];
>     var e = items.enumerate();
>     while (e.hasMoreElements()) {
>       let oldItem = e.getNext().QueryInterface(Ci.nsIJumpListShortcut);
>       if (oldItem) {
>         try { // in case we get a bad uri
>           let uriSpec = oldItem.app.getParameter(0);
>-          URIsToRemove.push(NetUtil.newURI(uriSpec));
>+          URIsToRemove.push(Services.io.newURI(uriSpec));
>         } catch (err) { }
>       }
>     }
Why these changes? Are they a ride along?
Other than that looks fine to me

Comment 14

[Frank-Rainer Grahl (:frg)]

This is bug 1364337. Just for alignment with Fx. Other than a few different formattings the files should now identical. If the rest is ok I would let theem stay in too.

Comment 15

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8878414 [details] [diff] [review]
1373565-CommitListBuild-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 1354143
User impact if declined: TB and SM crash
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): none.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878414 [details] [diff] [review]
1373565-CommitListBuild-V2.patch

You mean "beta", right, bug 1354143 landed in mozilla55, bug 692295 landed on mozilla56, some of the others in the mix on 55 or 56. The others are just cosmetic, so we'll just uplift this patch here.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Ian Neal from comment #13)
> Why these changes? Are they a ride along?
So to say: https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3#l1.98

Comment 18

[Frank-Rainer Grahl (:frg)]

>> You mean "beta", right,

Sorry not at home and I miss aurora... :) The crasher should be the first bug which needs an uplift. The rest can ride the trains. Cosmetics and needed bug 692295.

FRG

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/412abacb5ca5cdb65f08adfcae5846b275377d6b
https://hg.mozilla.org/comm-central/rev/e156549bd91a5036d58a7724dd386e041738cb46

Excellent work to track this down and assemble everything that's needed. Much appreciated!
Fingers crossed that the crashes in bug 1372522 are gone now.

The bad news is that we can toss away TB 55 beta we've just built. Ah well.

Comment 20

[Frank-Rainer Grahl (:frg)]

Great.

IanN if you don't want the Services.io.newURI change let me know and I will bake a backout fix for it.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Frank-Rainer Grahl (:frg) from comment #20)
> IanN if you don't want the Services.io.newURI change let me know and I will
> bake a backout fix for it.
We should have that hunk since we're following FF:
https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3#l1.98
They removed NetUtil a little further up
https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3#l1.43
so that Services.io.newURI is pretty much imperative - what's there not to like ;-)

Comment 22

[Frank-Rainer Grahl (:frg)]

Sorry set the flags incorrectly.

Comment 23

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+2) from comment #19)
> https://hg.mozilla.org/comm-central/rev/
> 412abacb5ca5cdb65f08adfcae5846b275377d6b
> https://hg.mozilla.org/comm-central/rev/
> e156549bd91a5036d58a7724dd386e041738cb46
> 
> Excellent work to track this down and assemble everything that's needed.
> Much appreciated!
> Fingers crossed that the crashes in bug 1372522 are gone now.

yup. Last nightly buildid with crashes is 2017-06-16 
https://crash-stats.mozilla.com/signature/?signature=CDestinationCategory%3A%3AReleaseReference&date=%3E%3D2017-06-12T12%3A29%3A00.000Z&date=%3C2017-06-19T12%3A29%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-date&page=1

Comment 24

[Jorg K (CEST = GMT+2)]

Beta (TB 55):
https://hg.mozilla.org/releases/comm-beta/rev/9b1fdf35d3e1cd80afb8866305125469e859faf3

Comment 25

[Ian Neal]

a=me for SM2.48 for whichever parts need uplifting

Comment 26

[Ian Neal]

(In reply to Ian Neal from comment #25)
> a=me for SM2.48 for whichever parts need uplifting

Or maybe that is for another bug :S