Closed Bug 1385090 Opened 3 years ago Closed 3 years ago

Pass Remote Pages instance from AboutNewTab on override

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: ahillier, Assigned: ahillier)

References

Details

Attachments

(1 file)

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: nobody → ahillier
Blocks: 1384240
Blocks: 1382139
No longer blocks: 1384240
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)
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.
Attachment #8891514 - Flags: review?(dtownsend)
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
Attachment #8891514 - Flags: review?(dtownsend)
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.
Attachment #8891514 - Flags: review?(dtownsend) → review+
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)
Summary: Add option for RemotePages to cache ports for specific URLs when it's destroyed → Pass Remote Pages instance from AboutNewTab on override
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c291fa5c4c8a
Pass Remote Pages instance from AboutNewTab on override r=mossop
Blocks: 1386785
https://hg.mozilla.org/mozilla-central/rev/c291fa5c4c8a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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?
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+
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.