Closed Bug 1472491 Opened 3 years ago Closed 3 years ago

Add tooling to lazily load content actors from JSMs based on events/messages/page loads

Categories

(Toolkit :: Async Tooling, enhancement)

Product:
Toolkit
Component:
Async Tooling
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Depends on 2 open bugs, Blocks 6 open bugs)

Details

(Whiteboard: [overhead:100k][cpstartup:8ms][qf:p3:f64])

Attachments

(45 files, 1 obsolete file)

Bug 1472491: Add [ChromeOnly] wantUntrusted event listener option.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1472491: Part 2a - Split matching logic for content scripts into MozDocumentMatcher base class.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1472491: Part 2b - Add MozDocumentObserver class to notify on new pattern-matched documents.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1472491: Part 3 - Fix flaky browser_ext_menus_events test.
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.
59 bytes, text/x-review-board-request
Felipe
: review+
mossop
: superreview+
Details
Bug 1472491: Part 4b - Add lazy actor support to browser_all_files_referenced.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1472491: Part 5a - Add BrowserTabChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5b - Add AboutReaderChild actor. f=felipe
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1472491: Part 5c - Add PageStyleChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5d - Add ContentSearchChild actor. f=felipe
59 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1472491: Part 5e - Add ContextMenuChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5f - Add ClickHandlerChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5g - Add PageInfoChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5h - Add LightWeightThemeChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5i - Add PluginChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5j - Add BlockedSiteChild actor. f=felipe
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1472491: Part 5k - Add NetErrorChild actor. f=felipe
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1472491: Part 5l - Add OfflineAppsChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5m - Add DOMFullscreenChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5n - Add AudioPlaybackChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5o - Add FindBarChild actor.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1472491: Part 5p - Add ExtFindChild actor. f=felipe
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1472491: Part 5q - Add SelectChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5r - Add PrintingChild actor.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5s - Add ZoomChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5t - Add ThumbnailsChild actor.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1472491: Part 5u - Add URIFixupChild actor.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1472491: Part 5v - Remove Browser:HideSessionRestoreButton listener.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1472491: Part 5w - Add PageMetadataChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5x - Add SelectionSourceChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5y - Add PopupBlockingChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5z - Add WebChannelChild actor. f=mconley
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1472491: Part 5α - Add DateTimePickerChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5β - Add ShieldFrameChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5γ - Add UITourChild actor.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1472491: Part 5δ - Add UnselectedTabHoverChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5ε - Add PurgeSessionHistoryChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5ζ - Add ControllersChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5η - Add BrowserChild actor. f=mconley
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1472491: Part 5θ - Add WebRTCChild actor. f=mconley
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1472491: Part 5ι - Add LoginManagerChild actor. f=mconley
59 bytes, text/x-review-board-request
Details
Bug 1472491: Part 5κ - Add ManifestMessagesChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 5λ - Add Split RemoteFinder into FinderChild and FinderParent actors.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1472491: Part 5μ - Add WebNavigationChild actor.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1472491: Part 0 - Process UpdateSharedData message in middle man process. r?mccr8
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review 
We've been working on reducing the amount of code that we load into content processes at startup, mostly by extracting special-purpose code into separate modules, and lazily loading it when it's needed. So far the work has mostly been ad-hoc, with special-purpose lazy loaders written for each module. But it's been useful for figuring out what kind of general-purpose lazy loaders we'll actually need.

Separately from this, but somewhat overlapping it, we need to change the way we handle IPC for remote browsers, largely so that we can cleanly handle out-of-process iframes. That's going to require concrete actors at the frame and process levels, rather than one actor per top-level-frame. And in order to handle that new model efficiently, lazy loading will become even more important.

In the relatively near future, all of these actors are going to need to be backed by native IPDL endpoints, and have defined sets of structured messages they can respond to.

However, as a first step toward that eventual goal, I'm going to implement these as simple JS objects that can send and receive arbitrary messages, which can eventually be refined into strictly defined protocol actors.
Whiteboard: [overhead:noted]
With my current prototype, this winds up saving about 100K.
Whiteboard: [overhead:noted] → [overhead:100k]
This winds up being 44 changesets, but most of them are super trivial. I mostly split them up in case anyone needs to bisect failures in the future.
Comment on attachment 8996524 [details]
Bug 1472491: Part 2b - Add MozDocumentObserver class to notify on new pattern-matched documents.

https://reviewboard.mozilla.org/r/260586/#review267678


Code analysis found 4 defects in this patch:
 - 4 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/WebExtensionPolicy.cpp:601
(Diff revision 1)
> +
> +void
> +DocumentObserver::Observe(const dom::Sequence<OwningNonNull<MozDocumentMatcher>>& matchers, ErrorResult& aRv)
> +{
> +  if (!EPS().RegisterObserver(*this)) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Error: You must immediately return after calling this function [clang-tidy: mozilla-must-return-from-caller]

    aRv.Throw(NS_ERROR_FAILURE);
    ^

::: toolkit/components/extensions/WebExtensionPolicy.cpp:601
(Diff revision 1)
> +
> +void
> +DocumentObserver::Observe(const dom::Sequence<OwningNonNull<MozDocumentMatcher>>& matchers, ErrorResult& aRv)
> +{
> +  if (!EPS().RegisterObserver(*this)) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Error: You must immediately return after calling this function [clang-tidy: mozilla-must-return-from-caller]

    aRv.Throw(NS_ERROR_FAILURE);
    ^

::: toolkit/components/extensions/WebExtensionPolicy.cpp:606
(Diff revision 1)
> +    aRv.Throw(NS_ERROR_FAILURE);
> +  }
> +  mMatchers.Clear();
> +  for (auto& matcher : matchers) {
> +    if (!mMatchers.AppendElement(matcher, fallible)) {
> +      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

Error: You must immediately return after calling this function [clang-tidy: mozilla-must-return-from-caller]

      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
      ^

::: toolkit/components/extensions/WebExtensionPolicy.cpp:606
(Diff revision 1)
> +    aRv.Throw(NS_ERROR_FAILURE);
> +  }
> +  mMatchers.Clear();
> +  for (auto& matcher : matchers) {
> +    if (!mMatchers.AppendElement(matcher, fallible)) {
> +      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

Error: You must immediately return after calling this function [clang-tidy: mozilla-must-return-from-caller]

      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
      ^
Comment on attachment 8996523 [details]
Bug 1472491: Part 2a - Split matching logic for content scripts into MozDocumentMatcher base class.

Apparently I already got review for these in bug 1425104 and forgot.
Attachment #8996523 - Flags: review?(tomica) → review+
Attachment #8996524 - Flags: review?(tomica) → review+
Comment on attachment 8996522 [details]
Bug 1472491: Add [ChromeOnly] wantUntrusted event listener option.

https://reviewboard.mozilla.org/r/260582/#review267680

::: dom/events/EventTarget.h:294
(Diff revision 1)
>    /**
>     * A method to compute the right wantsUntrusted value for AddEventListener.
>     * This will call the above hook as needed.
>     */
>    bool ComputeWantsUntrusted(const Nullable<bool>& aWantsUntrusted,
> +                             const AddEventListenerOptionsOrBoolean* aOptions,

Please document this argument (in particular what nullptr means).
Attachment #8996522 - Flags: review?(bzbarsky) → review+
Comment on attachment 8996564 [details]
Bug 1472491: Part 5λ - Add Split RemoteFinder into FinderChild and FinderParent actors.

https://reviewboard.mozilla.org/r/260666/#review267698

::: toolkit/modules/ActorManagerParent.jsm:159
(Diff revision 1)
>    FindBar: {
>      module: "resource://gre/modules/FindBar",
>      child: {
>        events: {
>          "keypress": {mozSystemGroup: true},
>        },
>      },
>    },
>  
> +  Finder: {
> +    module: "resource://gre/modules/Finder",
> +    child: {
> +      messages: [
> +        "Finder:Initialize",
> +      ],
> +    },
> +  },

It's a bit confusing we have both Finder and FindBar actors here. Follow-up bug to make that more sane?

::: toolkit/modules/moz.build
(Diff revision 1)
> -with Files('RemoteFinder.jsm'):
> -    BUG_COMPONENT = ('Toolkit', 'Find Toolbar')

Nit: add FinderChild/FinderParent.jsm as files here instead?
Attachment #8996564 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8996548 [details]
Bug 1472491: Part 5u - Add URIFixupChild actor.

https://reviewboard.mozilla.org/r/260634/#review267704

Ugh, we should really move uri fixup that involves the search engine to the parent process instead, but that's not something for this bug.
Attachment #8996548 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8996542 [details]
Bug 1472491: Part 5o - Add FindBarChild actor.

https://reviewboard.mozilla.org/r/260622/#review267714

This made my head hurt. Thanks for cleaning this up.

::: toolkit/modules/FindBarContent.jsm:14
(Diff revision 1)
>  ChromeUtils.defineModuleGetter(this, "RemoteFinder",
>                                 "resource://gre/modules/RemoteFinder.jsm");
>  ChromeUtils.defineModuleGetter(this, "Services",
>                                 "resource://gre/modules/Services.jsm");
>  
>  /* Please keep in sync with toolkit/this.mm.content/widgets/findbar.xml */

wat?
Attachment #8996542 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8996527 [details]
Bug 1472491: Part 4b - Add lazy actor support to browser_all_files_referenced.

https://reviewboard.mozilla.org/r/260592/#review267744

::: browser/base/content/test/static/browser_all_files_referenced.js:429
(Diff revision 1)
>          // Make urls like chrome://browser/skin/ point to an actual file,
>          // and remove the ref if any.
> +        try {
> -        url = Services.io.newURI(url).specIgnoringRef;
> +          url = Services.io.newURI(url).specIgnoringRef;
> +        } catch (e) {
> +          continue;

When do you expect this to throw? Could you please indicate it in a comment in the catch block?
Attachment #8996527 - Flags: review?(florian) → review+
Comment on attachment 8996527 [details]
Bug 1472491: Part 4b - Add lazy actor support to browser_all_files_referenced.

https://reviewboard.mozilla.org/r/260592/#review267744

> When do you expect this to throw? Could you please indicate it in a comment in the catch block?

It fails when trying to parse "chrome://mochitests/content/*"
Comment on attachment 8996564 [details]
Bug 1472491: Part 5λ - Add Split RemoteFinder into FinderChild and FinderParent actors.

https://reviewboard.mozilla.org/r/260666/#review267698

> It's a bit confusing we have both Finder and FindBar actors here. Follow-up bug to make that more sane?

Yeah, I was thinking that too.

> Nit: add FinderChild/FinderParent.jsm as files here instead?

We already have a pattern for `Finder*.jsm`
Comment on attachment 8996542 [details]
Bug 1472491: Part 5o - Add FindBarChild actor.

https://reviewboard.mozilla.org/r/260622/#review267714

> wat?

Er, sorry, search and replace fail...
Comment on attachment 8996547 [details]
Bug 1472491: Part 5t - Add ThumbnailsChild actor.

https://reviewboard.mozilla.org/r/260632/#review267820
Attachment #8996547 - Flags: review?(dtownsend) → review+
14:32:27 @Mossop | Does it take a 10% cut?

According to Treeherder, it (and the related patches from the last day or so) cut 150-200KB off of base content memory, and add about 12K of overhead from ActorManagerChild code... So, I guess the answer is approximately "yes".
Blocks: 1480244
This also shaves about 8ms off of cpstartup on Windows 64.
Whiteboard: [overhead:100k] → [overhead:100k][cpstartup:8ms][qf]
Blocks: 1480319
A quick note about possible wrapper's overhead.

I identified a regression on ContentFrameMessageManager that roots all message's `data` objects to the frame script global, instead of the global of the listener passed to `addMessageListener` (see bug 1474843). I imagine Sandboxes and JSM are impected in the same way by this regression.

So here, I'm expecting all `data` objects to be rooted to tab-content.js or browser-content.js frame script globals instead of `ActorManagerChild.jsm` global (where we do call `addMessageListener`). Then all IPC actor JSMs are going to have to go through crosscompartment wrappers whenever they access a `data` object.
So in all the patches where you move code from one of these two content scripts to a JSM, I'm expecting to see a regression due to bug 1474843 and crosscompartment wrappers. But if we fix it, then, I imagine we can pass `data` object around all JSM without having any wrapper, right? Because all JSMs share the same global?
Comment on attachment 8996526 [details]
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.

While I am reviewing the individual parts for correctness, the bug as a whole is a reasonably big shift into how the front-end code is written, so I don't want to be the sole responsible for giving it the green light. I think the overall approach needs a super-review or at least a second review by another peer.
Attachment #8996526 - Flags: superreview?(dtownsend)
(In reply to Alexandre Poirot [:ochameau] from comment #60)
> A quick note about possible wrapper's overhead.
> 
> I identified a regression on ContentFrameMessageManager that roots all
> message's `data` objects to the frame script global, instead of the global
> of the listener passed to `addMessageListener` (see bug 1474843). I imagine
> Sandboxes and JSM are impected in the same way by this regression.
> 
> So here, I'm expecting all `data` objects to be rooted to tab-content.js or
> browser-content.js frame script globals instead of `ActorManagerChild.jsm`
> global (where we do call `addMessageListener`). Then all IPC actor JSMs are
> going to have to go through crosscompartment wrappers whenever they access a
> `data` object.
> So in all the patches where you move code from one of these two content
> scripts to a JSM, I'm expecting to see a regression due to bug 1474843 and
> crosscompartment wrappers. But if we fix it, then, I imagine we can pass
> `data` object around all JSM without having any wrapper, right? Because all
> JSMs share the same global?

See bug 1480244. Getting rid of message manager globals is next on my todo list.

In the mean time, there may be some minor performance and memory regressions from the extra wrapper overhead, but it should be more than offset by the improvements we get from lazy loading and document filtering.
[Calling this a QF p3, tentatively targeting 64-or-sooner. Looking forward to 8ms faster content process startup! :)]
Whiteboard: [overhead:100k][cpstartup:8ms][qf] → [overhead:100k][cpstartup:8ms][qf:p3:f64]
(In reply to :Felipe Gomes (needinfo me!) from comment #61)
> Comment on attachment 8996526 [details]
> Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.
> 
> While I am reviewing the individual parts for correctness, the bug as a
> whole is a reasonably big shift into how the front-end code is written, so I
> don't want to be the sole responsible for giving it the green light. I think
> the overall approach needs a super-review or at least a second review by
> another peer.

Fair enough. Although, too be clear, this approach has been discussed repeatedly since we started project Fission, and we agreed on the broad details at the meeting in Toronto earlier this year, which included other peers, including ehsan and mconley. I didn't dive this far into it with no input :)
Comment on attachment 8996526 [details]
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.

https://reviewboard.mozilla.org/r/260590/#review268678

Unordered stream of thoughts:

- I feel that the list of messages/modules to be registered is kinda out-of-place living in nsBrowserGlue.js and ActorManagerParent.jsm.  It would be nice if there were two dedicated files to it (one in browser and another in toolkit), just for code organization and ease of documentation

With this, you could even not use the sharedData and just access these jsms directly in the child. It would simplify the code and get rid of the whole flush() stuff.
But I suspect you don't want to do that for memory purposes, right? If that's the case, then it's fine to keep the sharedData, but it doesn't break the point of having the lists in separate jsms.

- I do think it's important to have the whole filename of the actors present in the code (and not do the concatenation). It's a bit more verbose but it helps in productivity afterwards.  With the lists in a separate file with easy access to them, you can just write a test to enforce the naming convention that you want.

- It would be nice if all these actors being created were added to a new separate folder, browser/actors. What do you think?  browser/modules could be left for generic stuff, and then all the actors would be in browser/actors and toolkit/actors.


(Hmmm, even nicer if they mapped to resource:///actors/* . wdyt?)


- even though you already explained to me, I still didn't fully understand the difference to have the Singleton objects in the case where the matcher is used :P

::: browser/components/nsBrowserGlue.js:655
(Diff revision 1)
>      os.addObserver(this, "sync-ui-state:update");
>      os.addObserver(this, "handlersvc-store-initialized");
>      os.addObserver(this, "shield-init-complete");
>  
> +    ActorManagerParent.addActors(ACTORS);
> +    ActorManagerParent.flush();

this flush() feels like something that shouldn't be the responsibility of the caller.

There must be some notification that ActorManagerParent could listen to that gets called before the first child process is initiated

`final-ui-startup` comes to mind as a good candidate.

It would also be better as an API to document that addActors must be called before final-ui-startup, as that's more well defined. (vs. saying it must be called before a child process is initiated)

(and flush() can be left to dynamic callers that might need to call it afterwards, like in tests.. although I'd rename it to update())

::: toolkit/modules/ActorManagerParent.jsm:117
(Diff revision 1)
> +    this.observers = new DefaultMap(() => []);
> +  }
> +
> +  addActor(actorName, actor, module) {
> +    actorName += this.process;
> +    this.actors.set(actorName, {module: `${module}${this.process}.jsm`});

nit: reminder to add a comment here or change the variable name to make it clear this is not about process type
Attachment #8996526 - Flags: review?(felipc)
Comment on attachment 8996549 [details]
Bug 1472491: Part 5v - Remove Browser:HideSessionRestoreButton listener.

https://reviewboard.mozilla.org/r/260636/#review268984

Ah, this is from the old about:home, which was removed in bug 1409054, but I accidentally left this behind :P

So no need to create the AboutHomeChild here at all, just remove this message listener.


(note: since this was a very straightforward conversion, I linked this patch as an example in the e-mail I sent to firefox-dev)
Attachment #8996549 - Flags: review?(felipc) → review-
Comment on attachment 8996532 [details]
Bug 1472491: Part 5e - Add ContextMenuChild actor.

https://reviewboard.mozilla.org/r/260602/#review268994

::: browser/modules/ContextMenuChild.jsm:243
(Diff revision 1)
> +  static getTarget(global, message, key) {
> +    return contextMenus.get(global).getTarget(message, key);
> +  }
> +

we should somehow not forget to remove this if we add a way to get access to the actor's instance from outside
Attachment #8996532 - Flags: review?(felipc) → review+
Comment on attachment 8996526 [details]
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.

https://reviewboard.mozilla.org/r/260590/#review268990

Generally sr=me on the approach, but why is the flush so necessary?

::: toolkit/modules/ActorManagerChild.jsm:15
(Diff revision 1)
> + * ActorManagerParent.jsm for more information.
> + */
> +
> +var EXPORTED_SYMBOLS = ["ActorManagerChild"];
> +
> +ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");

IIRC this would work:

    const { ExtensionUtils: { DefaultMap }} = ...

::: toolkit/modules/ActorManagerChild.jsm:23
(Diff revision 1)
> +const {DefaultMap} = ExtensionUtils;
> +
> +const {sharedData} = Services.cpmm;
> +
> +function getMessageManager(window) {
> +  return window.docShell.QueryInterface(Ci.nsIInterfaceRequestor)

Update from bug 1479569
Attachment #8996526 - Flags: review+
Attachment #8996531 - Flags: review?(felipc)
Attachment #8996531 - Flags: review?(adw)
Attachment #8996531 - Flags: feedback+
Comment on attachment 8996533 [details]
Bug 1472491: Part 5f - Add ClickHandlerChild actor.

https://reviewboard.mozilla.org/r/260604/#review268996
Attachment #8996533 - Flags: review?(felipc) → review+
Comment on attachment 8996535 [details]
Bug 1472491: Part 5h - Add LightWeightThemeChild actor.

https://reviewboard.mozilla.org/r/260608/#review268998
Attachment #8996535 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #65)
> Comment on attachment 8996526 [details]
> Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.
> 
> https://reviewboard.mozilla.org/r/260590/#review268678
> 
> Unordered stream of thoughts:
> 
> - I feel that the list of messages/modules to be registered is kinda
> out-of-place living in nsBrowserGlue.js and ActorManagerParent.jsm.  It
> would be nice if there were two dedicated files to it (one in browser and
> another in toolkit), just for code organization and ease of documentation

I think this is the right place for them, for now. I eventually intend to add a
way to specify manifests in moz.build files, so we can aggregate them at compile
time, but in the mean time, I don't want to add more scripts that we have to
load in every session.

> With this, you could even not use the sharedData and just access these jsms
> directly in the child. It would simplify the code and get rid of the whole
> flush() stuff.

We can't load the lists directly in the child without a build step. I'll
probably add a build step at some point, but I'd rather avoid that for the
initial implementation, and particularly until I know more about how the final
JS IPDL and Fission actor APIs will look.

But in the mean time, we need to pre-process the actor definitions so that they
can be efficiently registered in each frame loader. The format they're specified
in is easy for humans to maintain, but not ideal for the registration code.
Using it directly would add about 2ms to each frame loader initialization (on a
fast machine). That would still be much faster than what we have now, but is
easily avoidable, so I'd rather avoid it.

> - It would be nice if all these actors being created were added to a new
> separate folder, browser/actors. What do you think?  browser/modules could
> be left for generic stuff, and then all the actors would be in
> browser/actors and toolkit/actors.
>
> (Hmmm, even nicer if they mapped to resource:///actors/* . wdyt?)

That would probably be sensible.

> - even though you already explained to me, I still didn't fully understand
> the difference to have the Singleton objects in the case where the matcher
> is used :P

The main difference is that the Singleton objects deal with a single actor
definition, rather than multiple actors, and are tied to single pages rather
than the entire life cycle of the frame loader. They're created when a matching
page is loaded, destroyed when that page is destroyed, and inactive when the
page is hidden.

> ::: browser/components/nsBrowserGlue.js:655
> (Diff revision 1)
> >      os.addObserver(this, "sync-ui-state:update");
> >      os.addObserver(this, "handlersvc-store-initialized");
> >      os.addObserver(this, "shield-init-complete");
> >  
> > +    ActorManagerParent.addActors(ACTORS);
> > +    ActorManagerParent.flush();
> 
> this flush() feels like something that shouldn't be the responsibility of
> the caller.
> 
> There must be some notification that ActorManagerParent could listen to that
> gets called before the first child process is initiated
> 
> `final-ui-startup` comes to mind as a good candidate.

There is not. This has to happen before we create any frame message manager,
which happens early, and does not have any corresponding notification.

I don't love the flush() call, but I think it's a reasonable compromise for the
initial implementation. In the future, I'd like us to be able to add dynamic
actor registrations, which will require adding some new infrastructure to the
frame loader code, but should make the explicit flushes unnecessary.

> It would also be better as an API to document that addActors must be called
> before final-ui-startup, as that's more well defined. (vs. saying it must be
> called before a child process is initiated)

final-ui-startup is irrelevant. It needs to happen before a frame message
manager is created.

(In reply to Dave Townsend [:mossop] from comment #68)
> Comment on attachment 8996526 [details]
> Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.
> 
> https://reviewboard.mozilla.org/r/260590/#review268990
> 
> Generally sr=me on the approach, but why is the flush so necessary?

Complicated. See above.

> ::: toolkit/modules/ActorManagerChild.jsm:15
> (Diff revision 1)
> > + * ActorManagerParent.jsm for more information.
> > + */
> > +
> > +var EXPORTED_SYMBOLS = ["ActorManagerChild"];
> > +
> > +ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
> 
> IIRC this would work:
> 
>     const { ExtensionUtils: { DefaultMap }} = ...

It would, but I generally avoid destructuring the return value of Cu.import,
since that's a module global which a) may expose symbols that we don't mean to
export, and b) won't include lexical declarations, which the module loader takes
special steps to be able to access.

I have plans to change ChromeUtils.import() to return an exports object rather
than the global by default, after which things like this will make more sense.
Comment on attachment 8996528 [details]
Bug 1472491: Part 5a - Add BrowserTabChild actor.

https://reviewboard.mozilla.org/r/260594/#review269000

::: browser/base/content/tab-content.js
(Diff revision 1)
>  DOMFullscreenHandler.init();
>  
> -var UserContextIdNotifier = {
> -  init() {
> -    addEventListener("DOMWindowCreated", this);
> -    this.init = null;

I hadn't seen https://reviewboard.mozilla.org/r/255640/#review262838 before, but I'm now wondering if this new approach doesn't revert the memory wins from that patch?

::: browser/base/content/tab-content.js
(Diff revision 1)
> -    // We use the docShell because content.document can have been loaded before
> -    // setting the originAttributes.

this comment sounds important but it wasn't kept in the new code location

::: browser/modules/BrowserTabChild.jsm:17
(Diff revision 1)
> +    switch (event.type) {
> +    case "DOMWindowCreated":

nit: missing indentation inside the switch block

::: browser/modules/BrowserTabChild.jsm:42
(Diff revision 1)
> +      break;
> +    }
> +  }
> +
> +  switchDocumentDirection(window = this.content) {
> +   // document.dir can also be "auto", in which case it won't change

nit: missing one space of indent in this comment

::: browser/modules/BrowserTabChild.jsm:60
(Diff revision 1)
> +        if (this.docShell) {
> +          this.docShell.isAppTab = message.data.isAppTab;
> +        }

the check for a null docshell was added in bug 1160104, because apparently session store could send this message too late.

Does the new lifetime management done by the actor manager guarantees that docshell is non-null?
Attachment #8996528 - Flags: review?(felipc) → review+
Comment on attachment 8996556 [details]
Bug 1472491: Part 5γ - Add UITourChild actor.

https://reviewboard.mozilla.org/r/260650/#review269002
Attachment #8996556 - Flags: review+
Comment on attachment 8996562 [details]
Bug 1472491: Part 5ι - Add LoginManagerChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260662/#review269004

::: toolkit/components/passwordmgr/LoginManagerChild.jsm:19
(Diff revision 1)
> +class LoginManagerChild extends ActorChild {
> +  constructor(mm) {
> +    super(mm);

Is there a reason this isn't part of LoginManagerContent.jsm instead? It seems like every event will cause it to get loaded anyways so the separation seems to only add unnecessary indirection AFAICT.
Attachment #8996562 - Flags: review?(MattN+bmo)
Attachment #8996546 - Flags: review?(felipc) → review?(mconley)
Attachment #8996550 - Flags: review?(felipc) → review?(mconley)
Attachment #8996551 - Flags: review?(felipc) → review?(mconley)
Attachment #8996552 - Flags: review?(felipc) → review?(mconley)
Attachment #8996553 - Flags: review?(felipc) → review?(mconley)
Attachment #8996554 - Flags: review?(felipc) → review?(mconley)
Attachment #8996555 - Flags: review?(felipc) → review?(mconley)
Attachment #8996556 - Flags: review?(felipc) → review?(mconley)
Attachment #8996557 - Flags: review?(felipc) → review?(mconley)
Attachment #8996558 - Flags: review?(felipc) → review?(mconley)
Attachment #8996559 - Flags: review?(felipc) → review?(mconley)
Attachment #8996560 - Flags: review?(felipc) → review?(mconley)
Attachment #8996561 - Flags: review?(felipc) → review?(mconley)
Attachment #8996562 - Flags: review?(felipc) → review?(mconley)
Attachment #8996563 - Flags: review?(felipc) → review?(mconley)
Attachment #8996565 - Flags: review?(felipc) → review?(mconley)
Comment on attachment 8996546 [details]
Bug 1472491: Part 5s - Add ZoomChild actor.

https://reviewboard.mozilla.org/r/260630/#review269008
Attachment #8996546 - Flags: review?(mconley) → review+
Attachment #8996553 - Flags: review?(mconley) → review?(markh)
Attachment #8996556 - Flags: review?(mconley)
Attachment #8996562 - Flags: review?(mconley)
Attachment #8996560 - Flags: review?(mconley) → review?(florian)
Attachment #8996561 - Flags: review?(mconley) → review?(florian)
Attachment #8996559 - Flags: review?(mconley) → review?(enndeakin)
Comment on attachment 8996562 [details]
Bug 1472491: Part 5ι - Add LoginManagerChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260662/#review269004

> Is there a reason this isn't part of LoginManagerContent.jsm instead? It seems like every event will cause it to get loaded anyways so the separation seems to only add unnecessary indirection AFAICT.

I don't remember the exact reason now. I think the two main reasons were that I was trying to avoid invasive changes in this bug (and LoginManagerContent didn't match the current actor model very well), and that these listeners also did somethings with InsecurePasswordUtils, which didn't seem to really fit in LoginManagerContent, but which I also couldn't find a particularly good way to split out.

There's also the possibility of splitting up/lazifying more of login manager content, though, and having a separate actor to handle lazily loading the parts that we need at any given moment would make that much easier. The DOMAutoComplete handler, for instance, involves code that mostly doesn't overlap with the logic required by the other handlers. And even when it's called, we can probably do some pre-filtering to prevent it from being loaded when we focus a non-username field.

That said, I don't have that strong an opinion on it, and if you'd rather this all go into LoginManagerContent, I'd be OK with that. But I may move it to a separate bug, in that case.
Comment on attachment 8996529 [details]
Bug 1472491: Part 5b - Add AboutReaderChild actor.  f=felipe

(since this is a large-ish chunk of code, could it be hg copied to preserve blame?)
Attachment #8996529 - Flags: review?(jaws)
Attachment #8996529 - Flags: review?(felipc)
Attachment #8996529 - Flags: feedback+
Comment on attachment 8996528 [details]
Bug 1472491: Part 5a - Add BrowserTabChild actor.

https://reviewboard.mozilla.org/r/260594/#review269000

> I hadn't seen https://reviewboard.mozilla.org/r/255640/#review262838 before, but I'm now wondering if this new approach doesn't revert the memory wins from that patch?

Sort of, sort of not.

The net effect is a major win, since we have many fewer of these init functions to begin with. And for actors that we load more than once, we come out ahead even for the init functions that we only call once, because we don't have to clone them into each message manager global before executing them. We kept a cached copy of those scripts in the shared JSM global, anyway, so we could re-use it when we executed the scripts the scripts in new frame loaders, so the nulling only got rid of the extra copy. In the new code, though, there is no extra copy.

For scripts that we *don't* load more than once, well, we have some provisions for throwing away the cached copy when we're pretty sure the script will only be loaded once, but that doesn't apply to any of the things this patch touches.

> this comment sounds important but it wasn't kept in the new code location

It sounds like nonsense to me, which is why I dropped it. If it makes more sense to you, I can perhaps re-word it, but the docShell is the obvious place to fetch origin attributes from, so I'm not convinced it would add any value even if re-worded.

> the check for a null docshell was added in bug 1160104, because apparently session store could send this message too late.
> 
> Does the new lifetime management done by the actor manager guarantees that docshell is non-null?

It guarantees that for singleton actors, since they go away when pages are hidden or destroyed (which happens when we tear down frame message managers), but not for other actors. We could manage that, by unregistering all listeners when we get an unload event on the message manager, but I'm not sure it's worth the overhead.
Comment on attachment 8996536 [details]
Bug 1472491: Part 5i - Add PluginChild actor.

https://reviewboard.mozilla.org/r/260610/#review269024
Attachment #8996536 - Flags: review?(felipc) → review+
Comment on attachment 8996562 [details]
Bug 1472491: Part 5ι - Add LoginManagerChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260662/#review269004

> I don't remember the exact reason now. I think the two main reasons were that I was trying to avoid invasive changes in this bug (and LoginManagerContent didn't match the current actor model very well), and that these listeners also did somethings with InsecurePasswordUtils, which didn't seem to really fit in LoginManagerContent, but which I also couldn't find a particularly good way to split out.
> 
> There's also the possibility of splitting up/lazifying more of login manager content, though, and having a separate actor to handle lazily loading the parts that we need at any given moment would make that much easier. The DOMAutoComplete handler, for instance, involves code that mostly doesn't overlap with the logic required by the other handlers. And even when it's called, we can probably do some pre-filtering to prevent it from being loaded when we focus a non-username field.
> 
> That said, I don't have that strong an opinion on it, and if you'd rather this all go into LoginManagerContent, I'd be OK with that. But I may move it to a separate bug, in that case.

I guess the LoginManagerContent member variables `__formFillService` and `_messages` would have to move to being set in the constructor in order to be a `class` but that's not too bad. InsecurePasswordUtils is already used from LoginManagerContent so that wouldn't be too bad to move inside LMContent either. I'm not sure what other problems you'll encounter so it's hard to weigh the costs but I think having both LoginManagerContent.jsm and LoginManagerChild.jsm is confusing so that's my main concern. If you could try merge them then I think that would be good.
Comment on attachment 8996562 [details]
Bug 1472491: Part 5ι - Add LoginManagerChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260662/#review269022

::: toolkit/components/passwordmgr/LoginManagerChild.jsm:53
(Diff revision 1)
> +    switch (message.name) {
> +    case "RemoteLogins:fillForm":

Nit: indent the `case`

::: toolkit/modules/ActorManagerParent.jsm:168
(Diff revision 1)
> +  LoginManager: {
> +    module: "resource://gre/modules/LoginManager",
> +    child: {
> +      messages: [
> +        "RemoteLogins:fillForm",
> +      ],
> +      events: {
> +        "DOMAutoComplete": {},
> +        "DOMFormHasPassword": {},
> +        "DOMInputPasswordAdded": {},
> +      },

Is this also going to be used for Android? If so, I think the Android listeners should be removed too.
Attachment #8996562 - Flags: review?(MattN+bmo)
Comment on attachment 8996534 [details]
Bug 1472491: Part 5g - Add PageInfoChild actor.

https://reviewboard.mozilla.org/r/260606/#review269026
Attachment #8996534 - Flags: review?(felipc) → review+
Comment on attachment 8996530 [details]
Bug 1472491: Part 5c - Add PageStyleChild actor.

https://reviewboard.mozilla.org/r/260598/#review269030
Attachment #8996530 - Flags: review?(felipc) → review+
Attachment #8996543 - Flags: review?(mdeboer)
Attachment #8996543 - Flags: review?(felipc)
Attachment #8996543 - Flags: feedback+
Comment on attachment 8996553 [details]
Bug 1472491: Part 5z - Add WebChannelChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260644/#review269034
Attachment #8996553 - Flags: review?(markh) → review+
Attachment #8996537 - Flags: review?(jhofmann)
Attachment #8996537 - Flags: review?(felipc)
Attachment #8996537 - Flags: feedback+
Attachment #8996538 - Flags: review?(jhofmann)
Attachment #8996538 - Flags: review?(felipc)
Attachment #8996538 - Flags: feedback+
Comment on attachment 8996544 [details]
Bug 1472491: Part 5q - Add SelectChild actor.

https://reviewboard.mozilla.org/r/260626/#review269036
Attachment #8996544 - Flags: review?(felipc) → review+
Comment on attachment 8996526 [details]
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.

https://reviewboard.mozilla.org/r/260590/#review269120

::: browser/components/BrowserComponents.manifest
(Diff revision 1)
>  
>  component {eab9012e-5f74-4cbc-b2b5-a590235513cc} nsBrowserGlue.js
>  contract @mozilla.org/browser/browserglue;1 {eab9012e-5f74-4cbc-b2b5-a590235513cc}
>  category app-startup nsBrowserGlue service,@mozilla.org/browser/browserglue;1 application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} application={aa3c5121-dab2-40e2-81ca-7ea25febc110}
>  component {d8903bf6-68d5-4e97-bcd1-e4d3012f721a} nsBrowserGlue.js
> -#ifndef MOZ_MULET

Can you file a bug (I couldn't find one) to remove MOZ_MULET from the tree?
Comment on attachment 8996529 [details]
Bug 1472491: Part 5b - Add AboutReaderChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260596/#review269124
Attachment #8996529 - Flags: review?(jaws) → review+
Comment on attachment 8996554 [details]
Bug 1472491: Part 5α - Add DateTimePickerChild actor.

https://reviewboard.mozilla.org/r/260646/#review269134
Attachment #8996554 - Flags: review?(mconley) → review+
Comment on attachment 8996563 [details]
Bug 1472491: Part 5κ - Add ManifestMessagesChild actor.

https://reviewboard.mozilla.org/r/260664/#review269136
Attachment #8996563 - Flags: review?(mconley) → review+
Comment on attachment 8996555 [details]
Bug 1472491: Part 5β - Add ShieldFrameChild actor.

https://reviewboard.mozilla.org/r/260648/#review269138
Attachment #8996555 - Flags: review?(mconley) → review+
Comment on attachment 8996539 [details]
Bug 1472491: Part 5l - Add OfflineAppsChild actor.

https://reviewboard.mozilla.org/r/260616/#review269142
Attachment #8996539 - Flags: review?(felipc) → review+
Comment on attachment 8996540 [details]
Bug 1472491: Part 5m - Add DOMFullscreenChild actor.

https://reviewboard.mozilla.org/r/260618/#review269144
Attachment #8996540 - Flags: review?(felipc) → review+
Comment on attachment 8996541 [details]
Bug 1472491: Part 5n - Add AudioPlaybackChild actor.

https://reviewboard.mozilla.org/r/260620/#review269148
Attachment #8996541 - Flags: review?(felipc) → review+
Comment on attachment 8996545 [details]
Bug 1472491: Part 5r - Add PrintingChild actor.

https://reviewboard.mozilla.org/r/260628/#review269152
Attachment #8996545 - Flags: review?(felipc) → review+
Comment on attachment 8996557 [details]
Bug 1472491: Part 5δ - Add UnselectedTabHoverChild actor.

https://reviewboard.mozilla.org/r/260652/#review269150

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1482242 to eventually combine this stuff with our warming infrastructure since it's so similar.
Attachment #8996557 - Flags: review?(mconley) → review+
Comment on attachment 8996552 [details]
Bug 1472491: Part 5y - Add PopupBlockingChild actor.

https://reviewboard.mozilla.org/r/260642/#review269166
Attachment #8996552 - Flags: review?(mconley) → review+
Comment on attachment 8996550 [details]
Bug 1472491: Part 5w - Add PageMetadataChild actor.

https://reviewboard.mozilla.org/r/260638/#review269168

It's a little bit of a shame that we're losing the blame by cutting-and-pasting the actors over to new files, but at this point, it'd be pretty cruel to ask you to do a file copy on the frame scripts and carve the Actors out. So I won't. :)
Attachment #8996550 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (:⚙️) from comment #97)
> It's a little bit of a shame that we're losing the blame by
> cutting-and-pasting the actors over to new files, but at this point, it'd be
> pretty cruel to ask you to do a file copy on the frame scripts and carve the
> Actors out. So I won't. :)

I can still do it easily enough with `hg cp -A`, but I generally avoided `hg cp` except for patches where I was copying a large amount of code, because it makes rebasing a pain in the ass. I can go through before I land and add the copy metadata to the rest, though. It shouldn't be a problem at that point.
Comment on attachment 8996558 [details]
Bug 1472491: Part 5ε - Add PurgeSessionHistoryChild actor.

https://reviewboard.mozilla.org/r/260654/#review269230

::: toolkit/modules/PurgeSessionHistoryChild.jsm:13
(Diff revision 1)
> +    if (message.name != "Browser:PurgeSessionHistory") {
> +      return;
> +    }

Are we being just extra cautious here? Why the check?
Attachment #8996558 - Flags: review?(mconley) → review+
Comment on attachment 8996565 [details]
Bug 1472491: Part 5μ - Add WebNavigationChild actor.

https://reviewboard.mozilla.org/r/260668/#review269234

::: toolkit/modules/ActorManagerParent.jsm:298
(Diff revision 1)
> +        "WebNavigation:GoBack",
> +        "WebNavigation:GoForward",
> +        "WebNavigation:GotoIndex",
> +        "WebNavigation:LoadURI",
> +        "WebNavigation:Reload",
> +        "WebNavigation:SetOriginAttributes",
> +        "WebNavigation:Stop",

Yay for starting a convention of putting these things in alphabetical order!
Attachment #8996565 - Flags: review?(mconley) → review+
Comment on attachment 8996551 [details]
Bug 1472491: Part 5x - Add SelectionSourceChild actor.

https://reviewboard.mozilla.org/r/260640/#review269236
Attachment #8996551 - Flags: review?(mconley) → review+
Attachment #8996560 - Flags: feedback?(mconley) → feedback+
Comment on attachment 8996561 [details]
Bug 1472491: Part 5θ - Add WebRTCChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260660/#review269238

::: browser/modules/moz.build
(Diff revision 1)
> -    'ContentWebRTC.jsm',
>      'ContextMenuChild.jsm',
>      'DOMFullscreenChild.jsm',
>      'ExtensionsUI.jsm',
>      'Feeds.jsm',
> -    'FormSubmitObserver.jsm',

This seems like an unrelated change.
Comment on attachment 8996561 [details]
Bug 1472491: Part 5θ - Add WebRTCChild actor.  f=mconley

Seems okay - though that FormSubmitObserver thing probably isn't supposed to be removed. I'm hoping florian can sign-off on this though since I'm pretty unfamiliar with the WebRTC code.
Attachment #8996561 - Flags: feedback?(mconley) → feedback+
Comment on attachment 8996562 [details]
Bug 1472491: Part 5ι - Add LoginManagerChild actor.  f=mconley

In general, this seems okay (beyond the things that MattN brought up). MattN should probably get the final call here.
Attachment #8996562 - Flags: feedback?(mconley) → feedback+
Comment on attachment 8996537 [details]
Bug 1472491: Part 5j - Add BlockedSiteChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260612/#review269254

This looks good, thank you!
Attachment #8996537 - Flags: review?(jhofmann) → review+
Comment on attachment 8996559 [details]
Bug 1472491: Part 5ζ - Add ControllersChild actor.

Enn is going on PTO, and I'm pretty certain my review here is sufficient.
Attachment #8996559 - Flags: review?(enndeakin)
Comment on attachment 8996538 [details]
Bug 1472491: Part 5k - Add NetErrorChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260614/#review269256

NetError code is so messy gah :|

Had a question about handling clicks there but it's nothing I need to approve again I think, r=me with that resolved.

Thanks!

::: browser/modules/ClickHandlerChild.jsm:32
(Diff revision 1)
>        return;
>      }
>  
>      // Handle click events from about pages
>      if (event.button == 0) {
> -      if (this.mm.AboutNetAndCertErrorListener.isAboutCertError(ownerDoc)) {
> +      if (ownerDoc.documentURI.startsWith("about:blocked")) {

I don't really understand this change. ClickHandlerChild is still receiving events for all about: pages, right? Don't we still want to return early for about:certerror and about:neterror here?

If this is correct please at least combine the two if-statements.

::: browser/modules/NetErrorChild.jsm:506
(Diff revision 1)
> +      frameDocShell && frameDocShell.QueryInterface(Ci.nsIWebNavigation);
> +      if (!frameDocShell || !this.isAboutCertError(frameDocShell.document)) {
> +        return;
> +      }
> +
> +      this.onCertErrorDetails(this.mm, msg, frameDocShell);

onCertErrorDetails does not seem to use its first argument, so you might as well remove it.
Attachment #8996538 - Flags: review?(jhofmann) → review+
Comment on attachment 8996561 [details]
Bug 1472491: Part 5θ - Add WebRTCChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260660/#review269238

> This seems like an unrelated change.

Urgh. Yeah, rebase botch.
Comment on attachment 8996558 [details]
Bug 1472491: Part 5ε - Add PurgeSessionHistoryChild actor.

https://reviewboard.mozilla.org/r/260654/#review269230

> Are we being just extra cautious here? Why the check?

It's mostly just informative, to be honest. I generally prefer all message/event/observer callbacks to have some reference to the thing they're listening for to make it clearer to people reading the code, and to make it easier to search for.

It could just be a comment rather than an explicit check, but comments tend to get out of sync, and they don't work as well with code search tools when searching for strings.
Comment on attachment 8996538 [details]
Bug 1472491: Part 5k - Add NetErrorChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260614/#review269256

Agreed, it is messy. Hopefully it will be a bit easier to maintain now that it's untangled from the rest of the content scripts, though.

> I don't really understand this change. ClickHandlerChild is still receiving events for all about: pages, right? Don't we still want to return early for about:certerror and about:neterror here?
> 
> If this is correct please at least combine the two if-statements.

It is still receiving events for all about pages, yes, but there's no reason to special case about:neterror or about:certerror here. The default click handler doesn't have any ill effects, and if it did, we'd want to handle them by having click handlers for those pages that just call preventDefault on the events.
Attachment #8996526 - Flags: review?(felipc)
Attachment #8996549 - Flags: review?(felipc)
Attachment #8996562 - Flags: review?(MattN+bmo)
Attachment #8996549 - Flags: review?(felipc)
Attachment #8996526 - Flags: review?(felipc)
Comment on attachment 8996531 [details]
Bug 1472491: Part 5d - Add ContentSearchChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260600/#review269294
Attachment #8996531 - Flags: review?(adw) → review+
Comment on attachment 8996549 [details]
Bug 1472491: Part 5v - Remove Browser:HideSessionRestoreButton listener.

https://reviewboard.mozilla.org/r/260636/#review269302

nit: please also remove this message being sent in browser.js
Attachment #8996549 - Flags: review?(felipc) → review+
Comment on attachment 8996526 [details]
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors.

https://reviewboard.mozilla.org/r/260590/#review269304
Attachment #8996526 - Flags: review?(felipc) → review+
This message is necessary to initialize the IPC sharedData structures in every
content process. If any JS code tries to access cpmm.sharedData before it has
been processed, the process crashes. As happens on OS-X record-replay tests
when trying to land these patches.
Comment on attachment 8996543 [details]
Bug 1472491: Part 5p - Add ExtFindChild actor.  f=felipe

https://reviewboard.mozilla.org/r/260624/#review269346
Attachment #8996543 - Flags: review?(mdeboer) → review+
Comment on attachment 8996561 [details]
Bug 1472491: Part 5θ - Add WebRTCChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260660/#review269372

Looks good to me. Note: this has pretty good test coverage in the browser/base/content/test/webrtc folder, so if tests still pass, I'm pretty confident this can't be dramatically broken.

::: browser/modules/moz.build
(Diff revision 2)
>      'ContentObservers.js',
>      'ContentSearch.jsm',
> -    'ContentWebRTC.jsm',
>      'ExtensionsUI.jsm',
>      'Feeds.jsm',
> -    'FormSubmitObserver.jsm',

Removing 'FormSubmitObserver.jsm' seems unrelated to this patch.
Attachment #8996561 - Flags: review?(florian) → review+
Comment on attachment 8996560 [details]
Bug 1472491: Part 5η - Add BrowserChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260658/#review269370

::: toolkit/actors/BrowserChild.jsm:16
(Diff revision 2)
> +ChromeUtils.defineModuleGetter(this, "BrowserUtils",
> +                               "resource://gre/modules/BrowserUtils.jsm");
> +
> +class BrowserChild extends ActorChild {
> +  handleEvent(event) {
> +    if (event.type == "DOMWindowClose") {

Is the event.isTrusted check no longer needed? If so, why?
Attachment #8996560 - Flags: review?(florian) → review+
Comment on attachment 8996560 [details]
Bug 1472491: Part 5η - Add BrowserChild actor.  f=mconley

https://reviewboard.mozilla.org/r/260658/#review269370

> Is the event.isTrusted check no longer needed? If so, why?

It is not, and it never was. Chrome event listeners never fire for untrusted events unless you ask for them.
This message is necessary to initialize the IPC sharedData structures in every
content process. If any JS code tries to access cpmm.sharedData before it has
been processed, the process crashes. As happens on OS-X record-replay tests
when trying to land these patches.
Comment on attachment 8999701 [details]
Bug 1472491: Part 0 - Process UpdateSharedData message in middle man process. r?mccr8

Andrew McCreight [:mccr8] has approved the revision.
Attachment #8999701 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/823dbd89637cf9c0847de7957932220bcc6905a0
Bug 1472491: Part 1 - Add [ChromeOnly] wantUntrusted event listener option. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/5479607a771acc685552de2b7f968c0c64cc2358
Bug 1472491: Part 2a - Split matching logic for content scripts into MozDocumentMatcher base class. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/761dcb3fa4e90fe6e500671ca8123c6dae96103e
Bug 1472491: Part 2b - Add MozDocumentObserver class to notify on new pattern-matched documents. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3649a09b92e57555e71e8c2f0b4308c18dc252b
Bug 1472491: Part 4a - Add helper classes for lazily loading JS IPC actors. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/80ebfef17da585c25ac4f73e8d78d38bd1662253
Bug 1472491: Part 4b - Add lazy actor support to browser_all_files_referenced. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/66c5f71e12c6df95c8e3e1e63083d59c7f77c7d8
Bug 1472491: Part 5a - Add BrowserTabChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d69dc796d1fec209805c1dc9b0875f1b3ef056
Bug 1472491: Part 5b - Add AboutReaderChild actor. r=jaws f=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/271ac4e75c900d5a41602ca7de1c1b9823385e34
Bug 1472491: Part 5c - Add PageStyleChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/13dc8b7b72f3fc5f774a00335f68c4815338ff07
Bug 1472491: Part 5d - Add ContentSearchChild actor. r=adw f=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec3a9d01b242ed4054a319f43a50dfbbaa6d99c
Bug 1472491: Part 5e - Add ContextMenuChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e08d7ed69ebdc3614ddd2d2a9547994e0a2f73
Bug 1472491: Part 5f - Add ClickHandlerChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/9359b425ceaf244112a155533cd1cc6877b22028
Bug 1472491: Part 5g - Add PageInfoChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/923c346f02128f4498a7ca102ec75b4ee1aef2fa
Bug 1472491: Part 5h - Add LightWeightThemeChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbc93e84af5cfc0ce57f3c0bf3189bfcf20af74
Bug 1472491: Part 5i - Add PluginChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/17440776f078aa0219f72e97ce88f0b7a1217b30
Bug 1472491: Part 5j - Add BlockedSiteChild actor. r=johannh f=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/0196bac4114b13c869f57cad22d922920a5b8180
Bug 1472491: Part 5k - Add NetErrorChild actor. r=johannh f=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/58587e163a5912cf56164a3f4af43a8d114913be
Bug 1472491: Part 5l - Add OfflineAppsChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc5ef9b4bf34df0e28be99e5736f6a744b96696
Bug 1472491: Part 5m - Add DOMFullscreenChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/750cdb02923bb544ca7b38e606e87c6d35e0d74b
Bug 1472491: Part 5n - Add AudioPlaybackChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c2735a9a17f0dc87946f564f3981ba671b06ce
Bug 1472491: Part 5o - Add FindBarChild actor. r=gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/733c55bf5fe8e6348ad2020a3a863d01c88ab2f3
Bug 1472491: Part 5q - Add SelectChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/cddc9e0db0d65c9d9c9a6a37ec489c59da50c929
Bug 1472491: Part 5r - Add PrintingChild actor. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb9468f5609918be8a635fec650f55ca89666c5
Bug 1472491: Part 5s - Add ZoomChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c87ed2a2a357f965f071bab78149d970a01e164
Bug 1472491: Part 5t - Add ThumbnailsChild actor. r=Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/7699b1ced009a10ec61806cd6fdbc339c304106d
Bug 1472491: Part 5u - Add URIFixupChild actor. r=gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e7ca5cf109f0db99b3179e62c073a090201218
Bug 1472491: Part 5v - Remove Browser:HideSessionRestoreButton listener. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/3925f3d433172e3edd02cf0dbd28a24d354627a3
Bug 1472491: Part 5w - Add PageMetadataChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/795a767014f36aad7a9d890b0d894053e89d6a65
Bug 1472491: Part 5x - Add SelectionSourceChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b635ae98f46f7431fd5e95ba38d5454e18bf7b9
Bug 1472491: Part 5y - Add PopupBlockingChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/868162b370c877ef1ac9c9129010bcfe914ad0ed
Bug 1472491: Part 5z - Add WebChannelChild actor. r=markh f=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f69e0977c46d9df90fa3284c2b57dd946f7d52
Bug 1472491: Part 5α - Add DateTimePickerChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/83ab8ab55813ca7a63155acdfddf629ffd4546b9
Bug 1472491: Part 5β - Add ShieldFrameChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/c209596f78c2a5401671dc34545bf3cd4e49dfee
Bug 1472491: Part 5γ - Add UITourChild actor. r=MattN

https://hg.mozilla.org/integration/mozilla-inbound/rev/b683542cac352cb84c409aa52827accc0669a856
Bug 1472491: Part 5δ - Add UnselectedTabHoverChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d586e4a214e2e03b7c596872691f57acf240dcb
Bug 1472491: Part 5ε - Add PurgeSessionHistoryChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/1194a1b4bf239c861da8f8f89ab5e8dacb68848c
Bug 1472491: Part 5ζ - Add ControllersChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3e2acebb2c832fbcc04ed64be7b9bb35941b0c
Bug 1472491: Part 5κ - Add ManifestMessagesChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e04b88dde43abfeb8a2ee3fe21c561a79f6c1e9
Bug 1472491: Part 5λ - Add Split RemoteFinder into FinderChild and FinderParent actors. r=gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7b959b74404be51b72134222c8be46e2d8ad5a9
Bug 1472491: Part 5μ - Add WebNavigationChild actor. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/100500156e86ffbdea5439283c394a3ddf6c4e15
Bug 1472491: Part 5p - Add ExtFindChild actor. r=mikedeboer f=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/4da0eca21e18abbef9cebc5f0744ddc9384f5c26
Bug 1472491: Part 5η - Add BrowserChild actor. r=florian f=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/8920abb9801dcf5a330485b7393dce3265487660
Bug 1472491: Part 5θ - Add WebRTCChild actor. r=florian f=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e6aadbb2299dd10a54759850d9cf6fc5b18585
Bug 1472491: Follow-up: Add bug component to actors/ directories. r=bustage,npotb CLOSED TREE
Comment on attachment 8996525 [details]
Bug 1472491: Part 3 - Fix flaky browser_ext_menus_events test.

https://reviewboard.mozilla.org/r/260588/#review269452
Attachment #8996525 - Flags: review?(tomica) → review+
Comment on attachment 8996523 [details]
Bug 1472491: Part 2a - Split matching logic for content scripts into MozDocumentMatcher base class.

https://reviewboard.mozilla.org/r/260584/#review269454
Attachment #8996523 - Flags: review?(tomica) → review+
Comment on attachment 8996524 [details]
Bug 1472491: Part 2b - Add MozDocumentObserver class to notify on new pattern-matched documents.

https://reviewboard.mozilla.org/r/260586/#review269456
Attachment #8996524 - Flags: review?(tomica) → review+
Attachment #8996526 - Flags: superreview?(dtownsend) → superreview+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ceb7952766db7caf602bc5f09427bb973cd1017
Bug 1472491: Follow-up: Add missing CC trace for document observer hashtable. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb79c9adc1a6c772550702399bdfb3d0d3417966
Bug 1472491: Part 0 - Process UpdateSharedData message in middle man process. r=mccr8
Blocks: 1483363
https://hg.mozilla.org/mozilla-central/rev/823dbd89637c
https://hg.mozilla.org/mozilla-central/rev/5479607a771a
https://hg.mozilla.org/mozilla-central/rev/761dcb3fa4e9
https://hg.mozilla.org/mozilla-central/rev/c3649a09b92e
https://hg.mozilla.org/mozilla-central/rev/80ebfef17da5
https://hg.mozilla.org/mozilla-central/rev/66c5f71e12c6
https://hg.mozilla.org/mozilla-central/rev/a7d69dc796d1
https://hg.mozilla.org/mozilla-central/rev/271ac4e75c90
https://hg.mozilla.org/mozilla-central/rev/13dc8b7b72f3
https://hg.mozilla.org/mozilla-central/rev/4ec3a9d01b24
https://hg.mozilla.org/mozilla-central/rev/f1e08d7ed69e
https://hg.mozilla.org/mozilla-central/rev/9359b425ceaf
https://hg.mozilla.org/mozilla-central/rev/923c346f0212
https://hg.mozilla.org/mozilla-central/rev/0fbc93e84af5
https://hg.mozilla.org/mozilla-central/rev/17440776f078
https://hg.mozilla.org/mozilla-central/rev/0196bac4114b
https://hg.mozilla.org/mozilla-central/rev/58587e163a59
https://hg.mozilla.org/mozilla-central/rev/1dc5ef9b4bf3
https://hg.mozilla.org/mozilla-central/rev/750cdb02923b
https://hg.mozilla.org/mozilla-central/rev/b1c2735a9a17
https://hg.mozilla.org/mozilla-central/rev/733c55bf5fe8
https://hg.mozilla.org/mozilla-central/rev/cddc9e0db0d6
https://hg.mozilla.org/mozilla-central/rev/2bb9468f5609
https://hg.mozilla.org/mozilla-central/rev/7c87ed2a2a35
https://hg.mozilla.org/mozilla-central/rev/7699b1ced009
https://hg.mozilla.org/mozilla-central/rev/a3e7ca5cf109
https://hg.mozilla.org/mozilla-central/rev/3925f3d43317
https://hg.mozilla.org/mozilla-central/rev/795a767014f3
https://hg.mozilla.org/mozilla-central/rev/9b635ae98f46
https://hg.mozilla.org/mozilla-central/rev/868162b370c8
https://hg.mozilla.org/mozilla-central/rev/28f69e0977c4
https://hg.mozilla.org/mozilla-central/rev/83ab8ab55813
https://hg.mozilla.org/mozilla-central/rev/c209596f78c2
https://hg.mozilla.org/mozilla-central/rev/b683542cac35
https://hg.mozilla.org/mozilla-central/rev/2d586e4a214e
https://hg.mozilla.org/mozilla-central/rev/1194a1b4bf23
https://hg.mozilla.org/mozilla-central/rev/1e3e2acebb2c
https://hg.mozilla.org/mozilla-central/rev/6e04b88dde43
https://hg.mozilla.org/mozilla-central/rev/c7b959b74404
https://hg.mozilla.org/mozilla-central/rev/100500156e86
https://hg.mozilla.org/mozilla-central/rev/4da0eca21e18
https://hg.mozilla.org/mozilla-central/rev/8920abb9801d
https://hg.mozilla.org/mozilla-central/rev/69e6aadbb229
https://hg.mozilla.org/mozilla-central/rev/2ceb7952766d
https://hg.mozilla.org/mozilla-central/rev/cb79c9adc1a6
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1483444
I see some perf improvements here:
== Change summary for alert #15029 (as of Wed, 15 Aug 2018 21:56:57 GMT) ==

Improvements:

 10%  tpaint osx-10-10 opt e10s stylo     291.09 -> 260.85

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15029
Blocks: 1483664
Blocks: 1483738
Attachment #8999491 - Attachment is obsolete: true
Depends on: 1483960
Depends on: 1488055
No longer depends on: 1488055
Depends on: 1499896
Depends on: 1519306
Depends on: 1520075
Depends on: 1520383
Depends on: 1521110
You need to log in before you can comment on or make changes to this bug.