1385090 - Pass Remote Pages instance from AboutNewTab on override

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Adam Hillier :ahillier]

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.

Comment 1

[Adam Hillier :ahillier]

Attachment: Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review commit: https://reviewboard.mozilla.org/r/162644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162644/

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Ed Lee :Mardak]

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

Comment 4

[Dave Townsend [:mossop]]

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

Comment 5

[Ed Lee :Mardak]

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 6

[Dave Townsend [:mossop]]

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 7

[Adam Hillier :ahillier]

Comment on attachment 8891514 [details]
Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162644/diff/1-2/

Comment 8

[Ed Lee :Mardak]

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 9

[Dave Townsend [:mossop]]

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 10

[Adam Hillier :ahillier]

Comment on attachment 8891514 [details]
Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162644/diff/2-3/

Comment 11

[Adam Hillier :ahillier]

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 12

[Ed Lee :Mardak]

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@[e-mail]/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 13

[Dave Townsend [:mossop]]

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.

Comment 14

[Adam Hillier :ahillier]

Comment on attachment 8891514 [details]
Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162644/diff/3-4/

Comment 15

[Adam Hillier :ahillier]

Comment on attachment 8891514 [details]
Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162644/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae8bd889b02d
Pass Remote Pages instance from AboutNewTab on override r=mossop

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 18

[Adam Hillier :ahillier]

Comment on attachment 8891514 [details]
Bug 1385090 - Pass Remote Pages instance from AboutNewTab on override

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162644/diff/5-6/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c291fa5c4c8a
Pass Remote Pages instance from AboutNewTab on override r=mossop

Comment 20

[[github robot]]

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 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c291fa5c4c8a

Comment 22

[Ed Lee :Mardak]

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 23

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4f47139f802

Comment 25

[Andrei Vaida [:avaida]]

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