Move ActivityStream instantiation and uninit into AboutNewTab

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Mardak, Assigned: ursula)

Tracking

(Blocks 1 bug)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

11 months ago
Basically doing what was described in bug 1447130 comment 2. This should probably happen after bug 1474406 as that version data comes from install.rdf -> startup options parameter.

To summarize, we'll use AboutNewTab.init to simulate an early bootstrap.startup call to wait for a delayed event to load ActivityStream module and instantiate (we can leave bootstrap.startup to also load and track module unload for now). Additionally have AboutNewTab.uninit trigger the instance's uninit.
Reporter

Comment 1

11 months ago
We could probably also as part of this bug move AboutNewTab from browser/modules to browser/components/newtab by updating EXTRA_JS_MODULES into moz.build
Reporter

Updated

11 months ago
Blocks: 1329955
Reporter

Updated

11 months ago
Blocks: 1474411
Assignee

Updated

11 months ago
Assignee: nobody → usarracini

Updated

11 months ago
Severity: normal → enhancement
Reporter

Updated

11 months ago
Depends on: 1472038
Assignee

Comment 2

11 months ago
I believe what I can also do now that we're moving the init to AboutNewtab.jsm, is remove all the stuff about the two RemotePages instance and have RemotePage register the about: pages in ActivityStreamMessageChannel.jsm . I can remove the existing pageListener stuff in AboutNewTab.jsm that was there for the old tiles and have one source of truth with RemotePages channel.
Reporter

Comment 3

11 months ago
Do we actually create RemotePages from ActivityStreamMessageChannel? I would think we only use the one from AboutNewTab (and it's the only one that adds about:welcome right now too…). Notably, RemotePages from AboutNewTab registers earlier than ActivityStream loading.
Assignee

Comment 4

11 months ago
Yeah we only use the RemotePages if we don't get one from AboutNewTab first (which, I don't know if will ever happens anymore since tiles isn't around), but I think we can consolidate that and just make one in ActivityStreamMessageChannel and just add about:welcome to that one. It happens earlier, but since we're initializing now in AboutNewTab, we're only moving it to slightly later. It would allow us to get rid of all this override/reset stuff, which would make things simpler. I tried a patch out and it seems to be working just fine.
Reporter

Comment 5

11 months ago
ahillier had to fix some issues with early about:newtab pages in bug 1385090 that added the override passing. I'm not sure if it's still necessary given that we simulate messages for existing pages now. I don't see tests specifically in that bug, so we might need some manual testing to verify the change is good ??

Is ./mach run about:newtab sufficient to test?
Flags: needinfo?(ahillier)
Reporter

Comment 6

11 months ago
Alternatively, we can keep `earlyRP = new RemotePages()` creation in AboutNewTab on the later startup notificaiton `new ActivityStream(earlyRP)`

Some reason I remember thinking we do want to leverage getting RemotePages started ASAP…
(In reply to Ed Lee :Mardak (PTO July 7-22) from comment #5)
> ahillier had to fix some issues with early about:newtab pages in bug 1385090
> that added the override passing. I'm not sure if it's still necessary given
> that we simulate messages for existing pages now. I don't see tests
> specifically in that bug, so we might need some manual testing to verify the
> change is good ??
> 
> Is ./mach run about:newtab sufficient to test?

I don't remember there being explicit automated testing of this issue. If the RP is being initialised only slightly later then I guess it probably won't be a problem.

I was definitely manually testing it by opening Firefox with lots of tabs (10+) all of which were about:newtab, then quiting and restarting and seeing if any pages got orphaned when they were brought back by session restore. I seem to recall being able to pretty reliably trigger the issue by doing that at the time.
Flags: needinfo?(ahillier)
Assignee

Comment 8

11 months ago
Interestingly, doing `./mach run about:newtab` with only one instance of RP being created in ASMessageChannel doesn't work but doing Adam's manual testing multiple times seems to work just fine. Doing what you suggested earlier Ed where we create RP in AboutNewTab.jsm and pass it into new ActivityStream() seems to work both with `./mach run about:newtab` and with the manual testing. It's a bit hairier because we've got to go in and add options in the constructor for Store and ActivityStreamMessageChannel. Which probably involves fixing some tests on our side as well. So it does seem like we (for some reason??) need RP to be initialized just slightly earlier than other parts of AS.

So we can either:
1. Leave all the RP stuff exactly as is (maybe do a follow up to try and consolidate stuff later if we want) or
2. Do what you suggested, Ed, with passing RP along into Activity Stream

What do you think?
Reporter

Comment 9

11 months ago
We can just keep the existing RemotePages behavior for now. There will probably be more than this change that can be optimized when we're a component, so not sure what's a good way to track it, but the cleanup would probably help bug 1439892 some too. I wonder if startup related improvements should block bug 1432587 meta?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 13

11 months ago
mozreview-review
Comment on attachment 8991940 [details]
Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab

https://reviewboard.mozilla.org/r/256848/#review263830

Pretty sure not doing the add-on-specific startup/install behavior in AboutNewTab (i.e., unconditionally observing instead of checking startingUp) will fix the startup test failures.

::: browser/modules/AboutNewTab.jsm:41
(Diff revision 2)
>      this.pageListener = pageListener || new RemotePages(["about:home", "about:newtab", "about:welcome"]);
> -    this.pageListener.addMessageListener("NewTab:Customize", this.customize);
> -    this.pageListener.addMessageListener("NewTab:MaybeShowMigrateMessage",
> -      this.maybeShowMigrateMessage);
> +
> +    if (Services.startup.startingUp) {
> +      Services.obs.addObserver(this, BROWSER_READY_NOTIFICATION);
> +    } else {
> +      this.onBrowserReady();

These code paths were to support add-on installation after startup. Because AboutNewTab.init happens later than startingUp, onBrowserReady gets called immediately instead of waiting for the notification. This is what is causing activity-stream/places to load too early.

We don't need the check for startingUp and probably don't need the waitingForBrowserReady stuff either (as I believe that was more for handling very quick pref changes that turned activity stream off and on again).

::: browser/modules/AboutNewTab.jsm:107
(Diff revision 2)
>      return null;
>    },
>  
>    reset(pageListener) {
>      this.isOverridden = false;
>      this.init(pageListener);

Until we clean up the overridden stuff, looks like `init` can be called later, so we probably want to only add the sessionstore observer when init is called with no arguments.
Attachment #8991940 - Flags: review?(edilee) → review-
Comment hidden (mozreview-request)
Reporter

Comment 15

10 months ago
mozreview-review
Comment on attachment 8991940 [details]
Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab

https://reviewboard.mozilla.org/r/256848/#review264046

Looks to be passing tests! At least fix the removeObserver exception.

::: browser/modules/AboutNewTab.jsm:29
(Diff revision 3)
>  
> +  activityStream: null,
> +
> +  /**
> +   * init - Initializes an instance of ActivityStream. This could be called directly
> +   *        in browser.js or once the browser is ready via BROWSER_READY_NOTIFICATION

This comment doesn't seem quite right. `init` isn't actually called from browser.js or via BROWSER_READY_NOTIFICATION.

Probably should describe pageListener parameter behavior too.

::: browser/modules/AboutNewTab.jsm:47
(Diff revision 3)
> -    this.pageListener.addMessageListener("NewTab:MaybeShowMigrateMessage",
> -      this.maybeShowMigrateMessage);
>    },
>  
> -  maybeShowMigrateMessage({ target }) {
> -    AutoMigrate.shouldShowMigratePrompt(target.browser).then((prompt) => {
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
> +  observe(subject, topic, data) {

nit: move the QI to probably the top and perhaps label the sections of the different "interfaces"

```
var AboutNewTab = {
  QueryInterface…
  
  // AboutNewTab
  
  pageListener…
  
  …
  
  // nsIObserver
  
  observe…
};
```

::: browser/modules/AboutNewTab.jsm:85
(Diff revision 3)
>      if (this.pageListener) {
>        this.pageListener.destroy();
>        this.pageListener = null;
>      }
> +
> +    Services.obs.removeObserver(this, BROWSER_READY_NOTIFICATION);

This is unnecessary and causes

14:50:52     INFO -  GECKO(8188) | JavaScript error: resource:///modules/AboutNewTab.jsm, line 85: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
Attachment #8991940 - Flags: review?(edilee) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

10 months ago
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f98112e49582
Move ActivityStream instantiation and uninit into AboutNewTab r=Mardak

Comment 19

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f98112e49582
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Comment 20

10 months ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/6848522edab49a1af51e7b748d96734da6f6f5ab
chore(mc): Port  Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab r=Mardak  (#4242)
Reporter

Updated

10 months ago
Iteration: 63.3 - Aug 6 → 63.2 - July 23
You need to log in before you can comment on or make changes to this bug.