Pass Remote Pages instance from AboutNewTab on override

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
General
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Adam Hillier, Assigned: Adam Hillier)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → ahillier

Updated

4 months ago
Blocks: 1384240

Updated

4 months ago
Blocks: 1382139
No longer blocks: 1384240
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8891514 - Flags: review?(dtownsend)
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.
Flags: needinfo?(ahillier)

Comment 3

4 months ago
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.

Comment 5

4 months ago
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

4 months ago
mozreview-review
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.
Attachment #8891514 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)

Comment 8

4 months ago
mozreview-review
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

4 months ago
mozreview-review
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
Attachment #8891514 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

4 months ago
mozreview-review-reply
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

4 months ago
mozreview-review-reply
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 13

4 months ago
mozreview-review
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.
Attachment #8891514 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

4 months ago
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)
Comment hidden (mozreview-request)

Updated

4 months ago
Summary: Add option for RemotePages to cache ports for specific URLs when it's destroyed → Pass Remote Pages instance from AboutNewTab on override

Comment 19

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

Comment 20

4 months ago
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)

Updated

4 months ago
Blocks: 1386785

Comment 21

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c291fa5c4c8a
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 22

4 months ago
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
Attachment #8891514 - Flags: approval-mozilla-beta?

Updated

4 months ago
status-firefox56: --- → affected
(Assignee)

Updated

4 months ago
Flags: needinfo?(ahillier)
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.
Attachment #8891514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/d4f47139f802
status-firefox56: affected → fixed

Updated

4 months ago
Blocks: 1387852
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.