In ActivityStream we have an issue where if an "about:newtab" page loads too early (which can happen during browser session restore) the ActivityStream instance of RemotePages doesn't create a port for it and so it's orphaned from ActivityStream message passing and breaks. (See https://github.com/mozilla/activity-stream/issues/2819) AboutNewTab.jsm is loaded early in the browser start-up and registers an instance of RemotePages for the url "about:newtab". When ActivityStream is initialised it calls AboutNewTab.override() which destroys the old RemotePages instance, before creating its own (also for "about:newtab"). Any "about:newtab" pages which had loaded before the override have child ports that are owned by the old instance and are destroyed when it is. The new instance is initialised with no child ports and as the pages have already loaded no new ports for them get created. Thus messages broadcast from Activity Stream don't reach those pages. Adding the ability for RemotePages clients to indicate that they'd like ports to be stored in a cache when the instance is destroyed and for subsequent instances initialised for the same url to have their messagePorts initialised with the cached ports would fix this issue.

How about instead of adding this complexity to RemotePageManager we instead just make AboutNewTab clear and return its RemotePages instance in the override function? This way activity stream gets the already set up RemotePages for any existing tabs that are loaded.

Oh that sounds pretty clever! Looks like we'll need to clean up any added message listeners when we "destroy" -> unoverride? https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm#131-137

(In reply to Ed Lee :Mardak from comment #3) > Oh that sounds pretty clever! Looks like we'll need to clean up any added > message listeners when we "destroy" -> unoverride? > > https://searchfox.org/mozilla-central/source/browser/extensions/activity- > stream/lib/ActivityStreamMessageChannel.jsm#131-137 Yeah that's right.

ahillier: you'll probably want to just directly make the changes in a m-c patch, and we'll backport the ActivityStreamMessageChannel.jsm changes to github.

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168528 As discussed in the bug waiting on a new patch here.

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168834 ::: browser/modules/AboutNewTab.jsm:45 (Diff revisions 1 - 2) > AutoMigrate.shouldShowMigratePrompt(target.browser).then((prompt) => { > if (prompt) { > AutoMigrate.showUndoNotificationBar(target.browser); > } > }); > - }, > + }).bind(this), Pretty sure you want to `.bind(AboutNewTab)` as `this` would be the top level scope.

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168832 Thanks, I really prefer this approach. Just a couple of things... ::: browser/base/content/test/newtab/head.js:90 (Diff revision 2) > gBrowser.contentWindow[prop] = oldSize[prop]; > } > }); > > Services.prefs.clearUserPref(PREF_NEWTAB_ENABLED); > - Services.prefs.clearUserPref(PREF_NEWTAB_ACTIVITY_STREAM); > + Services.prefs.setBoolPref(PREF_NEWTAB_ACTIVITY_STREAM, false); Is this change intentional? ::: browser/modules/AboutNewTab.jsm:45 (Diff revision 2) > AutoMigrate.shouldShowMigratePrompt(target.browser).then((prompt) => { > if (prompt) { > AutoMigrate.showUndoNotificationBar(target.browser); > } > }); > - }, > + }).bind(this), The function never uses this so binding seems to be unnecessary. ::: browser/modules/AboutNewTab.jsm:50 (Diff revision 2) > - }, > + }).bind(this), > > - customize(message) { > + customize: (function (message) { > NewTabUtils.allPages.enabled = message.data.enabled; > NewTabUtils.allPages.enhanced = message.data.enhanced; > - }, > + }).bind(this), Ditto here

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168832 > Is this change intentional? Yes the old line is the incorrect way of turning off AS and causes test failures.

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168832 > Yes the old line is the incorrect way of turning off AS and causes test failures. The `clearUserPref` is to restore the pref to the default state (true on nightly) to undo the setting to `false` at the beginning of the test: https://searchfox.org/mozilla-central/source/browser/base/content/test/newtab/head.js#9 This change makes the line do nothing as it's already false. The stack you provided earlier indicates activity-stream is trying to re-init (as the pref is cleared/set to `true` again): ``` 333 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: resource://gre/modules/RemotePageManager.jsm :: sendAsyncMessage :: line 259" data: no]"] sendAsyncMessage@resource://gre/modules/RemotePageManager.jsm:259:5 sendAsyncMessage@resource://gre/modules/RemotePageManager.jsm:130:7 broadcast@resource://activity-stream/lib/ActivityStreamMessageChannel.jsm:91:5 … onPrefChanged@activity-stream@mozilla.org/bootstrap.js:91:5 observe@resource://gre/modules/Preferences.jsm:409:9 @chrome://mochitests/content/browser/browser/base/content/test/newtab/head.js:90:3 ``` This seems to indicate some reason RemotePage isn't actually ready for some reason. Perhaps there's some failure in passing RemotePage from AboutNewTab --enableAS--> ActivityStreamMessageChannel --disableASForTest--> ANT --enableASTestEnd--> ASMC.

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override https://reviewboard.mozilla.org/r/162644/#review168866 ::: browser/modules/AboutNewTab.jsm:39 (Diff revision 3) > - this.pageListener.addMessageListener("NewTab:Customize", this.customize.bind(this)); > + this.pageListener.addMessageListener("NewTab:Customize", this.customize); > this.pageListener.addMessageListener("NewTab:MaybeShowMigrateMessage", > - this.maybeShowMigrateMessage.bind(this)); > + this.maybeShowMigrateMessage); > }, > > - maybeShowMigrateMessage({ target }) { > + maybeShowMigrateMessage: function ({ target }) { No need to change this line now. ::: browser/modules/AboutNewTab.jsm:47 (Diff revision 3) > AutoMigrate.showUndoNotificationBar(target.browser); > } > }); > }, > > - customize(message) { > + customize: function (message) { Ditto.

Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ae8bd889b02d Pass Remote Pages instance from AboutNewTab on override r=mossop

Backed out for eslint failures in AboutNewTab.jsm and RemotePageManager.jsm: https://hg.mozilla.org/integration/autoland/rev/72eb1ebe870483156e1ffe79ea43273c881178fa Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ae8bd889b02d932210c1dd101ddd39a3619bc3cc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120045836&repo=autoland TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/modules/AboutNewTab.jsm:70:7 | Method 'override' expected no return value. (consistent-return) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/modules/RemotePageManager.jsm:132:7 | Closing curly brace does not appear on the same line as the subsequent block. (brace-style)

Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c291fa5c4c8a Pass Remote Pages instance from AboutNewTab on override r=mossop

Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/b75fd530cc52adeecdf55da08168611d53a0d997 chore(mc): Backport bug 1385090 to borrow AboutNewTab RemotePages (#3063)

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override Approval Request Comment (part 4 of 6 Mardak will uplift) [Feature/Bug causing the regression]: Activity Stream preffed off in 56 to be enabled in Shield study [User impact if declined]: Users could start Firefox and find new tabs that say "undefined" [Is this code covered by automated tests?]: Yes, newtab tests cover the switching between Activity Stream on and off [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: Bug 1385716 Bug 1386265 Bug 1386350 Bug 1386314 Bug 1386737 [Is the change risky?]: No [Why is the change risky/not risky?]: The feature is the consumer of the api and is preffed off [String changes made/needed]: No

Comment on attachment 8891514 [details] Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override Part of a last minute uplift for Activity Stream testing in 56. This should make it into 56 beta 1.

(In reply to Ed Lee :Mardak from comment #22) > [Is this code covered by automated tests?]: Yes, newtab tests cover the > switching between Activity Stream on and off > [Has the fix been verified in Nightly?]: Yes > [Needs manual test from QE? If yes, steps to reproduce]: No Setting qe-verify- based on Ed Lee's assessment on manual testing needs and the fact that this fix has automated coverage.