1592776 - Fix dom/notification/test/mochitest/test_notification_tag.html for fission

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P2)

Description

[David Durst [:ddurst]]

Currently dom/notification/test/mochitest/test_notification_tag.html fails on opt and debug.

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 2

[Chris Peterson [:cpeterson]]

Matt, this bug is a blocker for Fission's current milestone (M4.1 aka "fix all the mochitests"), but it's currently unassigned. The Fission team is hoping teams will fix their mochitests for Fission before the end of Q1 (75 or 76 Nightly).

Will your team be able to prioritize this bug for Q1? If you don't think this mochitest failure should block shipping Fission, just let me know.

If you have questions for Fission engineers, you can reach them in the #fission channel on Slack or Riot.

Comment 3

[Matthew N. [:MattN]]

Logs:

0:06.96 TEST_START: dom/notification/test/mochitest/test_notification_tag.html
0:08.02 GECKO(31427) JavaScript error: resource://gre/modules/ProcessSelector.jsm, line 56: TypeError: can't access property "tabCount", process is null
0:08.02 GECKO(31427) JavaScript error: resource://gre/modules/ProcessSelector.jsm, line 56: TypeError: can't access property "tabCount", process is null
0:08.20 GECKO(31427) 2020-05-18 14:32:06.926 plugin-container[31435:1431313] NSNotificationCenter connection invalid

I'm not sure the above are relevant as the first issue I see is that the mockAlertsService has no effect. I thought the alert service was in the parent process so I'm not sure how this works with e10s either… I guess it was creating a second instance there… It seems like the test should be mocking the alert service in the parent process using something like SpecialPowers.loadChromeScript.

Moving to the DOM team since this isn't a notification front-end issue.

Comment 4

[Chris Peterson [:cpeterson]]

Jens, this notifications Fission bug needs an assignee.

The Fission team hopes to roll out Fission to some Nightly users in Q3, so it would be good if all tests are passing and enabled for Fission by the end of Q2.

Comment 5

[Jens Stutte [:jstutte]]

Simon, would you mind to take a look? Thank you!

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1592776 - Reformat test_notification_tag.html. r=#dom-workers-and-storage

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1592776 - Make test_notification_tag work with Fission. r=#dom-workers-and-storage,cpeterson

Depends on D76823

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa836c2449d5
Reformat test_notification_tag.html. r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/edaf8a5f8d53
Make test_notification_tag work with Fission. r=dom-workers-and-storage-reviewers,asuth

Comment 9

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1592776) for Mochitest failures on dom/notification/test/mochitest/test_notification_tag.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303899637&repo=autoland&lineNumber=4905

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=edaf8a5f8d5364db88956636a24f6623b05b5f0d

Backout:
https://hg.mozilla.org/integration/autoland/rev/7206207af8c4a14e959d05df93995d27873f21b6

Comment 10

[Simon Giesecke [:sg] [he/him]]

I can't seem to reproduce this locally, even with --verify. It's worth noting that this happened only on a Linux WebRender job, though I can't see any relation of this test to rendering.

Comment 11

[Dorel Luca [:dluca]]

Happens on Windows 7 debug and a other platforms:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Wo9Mh7OgSdq6wU0hJyjKPg-0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&fromchange=17e9d170ba1bd47cee22a7d0c17d2fb9941ae9c7&searchStr=mochitest%2C5

Comment 12

[Simon Giesecke [:sg] [he/him]]

Ah, I see. I only noticed the failure on the push itself. Then I'll try more to reproduce it.

Comment 13

[Simon Giesecke [:sg] [he/him]]

I was able to reproduce it now. It seems it's not a problem of the test_notification_tag.html test itself, but a side effect from an earlier test. When running test_notification_basics.html or test_notification_storage.html before, I can reproduce the problem. I have a Pernosco session for that at https://pernos.co/debug/BOs1d_DKqSfCQaew3YdXIQ/index.html#f{m[BtJZ,IpEK_,t[SA,V10_,f{e[BtJZ,IpEK_,s{af2PST4AA,bAYc,ojdSI,uiF2r___

The earlier test register a MockAlertsService in the content process, which causes the StaticComponents entry for the alerts service to be set to invalid from https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/xpcom/components/nsComponentManager.cpp#1610

Later, UnregisterFactory is called for the MockAlertsService, but the StaticComponents entry it is never set back to valid.

This would only happen from https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/xpcom/components/nsComponentManager.cpp#1587 within another RegisterFactory call under some conditions, but there is no further RegisterFactory call in the content process, because test_notification_tag.html now registers a mock service in the parent process.

It seems to me that UnregisterFactory should also set back the StaticComponents entry back to valid.

Kris, it seems you have last changed the code around the invalidation in Bug 1478124, what do you think?

Comment 14

[Kris Maglione [:kmag]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #13)

It seems to me that UnregisterFactory should also set back the StaticComponents entry back to valid.

Kris, it seems you have last changed the code around the invalidation in Bug 1478124, what do you think?

No, if a factory is unregistered, the only thing that should re-register it is another registerFactory call. The usual practice for tests that register these sorts of mock services is to call registerFactory again when they're done with the original CID and null for the factory object, which will cause it to map the original factory for the component CID to the given contract ID.

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1592776 - Fix cleanup of mock service registration. r=#dom-workers-and-storage

Comment 16

[Simon Giesecke [:sg] [he/him]]

(In reply to Kris Maglione [:kmag] from comment #14)

(In reply to Simon Giesecke [:sg] [he/him] from comment #13)

It seems to me that UnregisterFactory should also set back the StaticComponents entry back to valid.

Kris, it seems you have last changed the code around the invalidation in Bug 1478124, what do you think?

No, if a factory is unregistered, the only thing that should re-register it is another registerFactory call. The usual practice for tests that register these sorts of mock services is to call registerFactory again when they're done with the original CID and null for the factory object, which will cause it to map the original factory for the component CID to the given contract ID.

Ok, I now added https://phabricator.services.mozilla.com/D77781 to address that. It is still somewhat puzzling that unregisterFactory does not restore the state before registerFactory. I read between the lines that this is "only" relevant for tests, though?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9af9690fcef1
Fix cleanup of mock service registration. r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/e0a47494eaea
Reformat test_notification_tag.html. r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/052839fb9b56
Make test_notification_tag work with Fission. r=dom-workers-and-storage-reviewers,asuth

Comment 18

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (Bug 1592776) for mochitest failures at test_notification_basics.html.
https://hg.mozilla.org/integration/autoland/rev/4f7f6d6bc6183c2d5d12d8cc4d9419aeeecb32dd

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=052839fb9b56060e603e4fada1c8f7e56df4ae0f&selectedTaskRun=I91it6oRTkaiE1WJfxSp1g-0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304754735&repo=autoland&lineNumber=6530

Comment 19

[Narcis Beleuzu [:NarcisB]]

Please also check:

1. test_notification.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304759170&repo=autoland&lineNumber=15869
2. test_notification_constructor_error.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304757393&repo=autoland&lineNumber=7606

Comment 20

[Simon Giesecke [:sg] [he/him]]

This try run looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0d2210648bfd92bdff7483c67b567ea34176ad6

I will re-land this. Hope it's fine eventually.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/001e62003beb
Fix cleanup of mock service registration. r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/5cdf8bc8c919
Reformat test_notification_tag.html. r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/2b663e61ccba
Make test_notification_tag work with Fission. r=dom-workers-and-storage-reviewers,asuth

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/001e62003beb
https://hg.mozilla.org/mozilla-central/rev/5cdf8bc8c919
https://hg.mozilla.org/mozilla-central/rev/2b663e61ccba