1186843 - Notify observers "message-manager-will-change" before switching processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

As part of implementing process switching, nsFrameLoader will be changed to use different TabParent for different processes. Many js code uses frameLoader.messageManager for communication but changing TabParent means the underlying channel is also changed. I think we should notify the observers "message-manager-will-change" then disconnect the old message manager and create a new one then maybe notify them "message-manager-changed".

Comment 1

[Ethan Tseng [:ethan]]

Hi Kan-Ru,
We are doing triage for unassigned 2.5+ bugs.
Would you take this bug?

Comment 3

[Mahendra Potharaju [:mahe]]

Hey Kan-Ru, This bug blocks 2.5 and there hasn't been much action on this lately. Can you please find sometime for this? Thanks

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

I'm working on it.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Don't recreate message-manager

Looks like this is sufficient for now.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8658243 [details] [diff] [review]
Don't recreate message-manager

I don't see how this ends up loading pending scripts.
nsFrameMessageManager::InitWithCallback will just return early, right?
So, shouldn't we somehow force ReallyLoadFrameScripts to (re-)load frame scripts, since the child side of message manager has changed?

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8658243 [details] [diff] [review]
> Don't recreate message-manager
> 
> I don't see how this ends up loading pending scripts.
> nsFrameMessageManager::InitWithCallback will just return early, right?
> So, shouldn't we somehow force ReallyLoadFrameScripts to (re-)load frame
> scripts, since the child side of message manager has changed?

Yes, it won't load pending scripts. If I understand correctly, only global mm or broadcast mm should load pending scripts when it's connected to a new frame. For frame mm, pending scripts is only used when LoadFrameScript is called before InitWithCallback. So if we force ReallyLoadFrameScripts to (re-)load frame scripts, it would only load part of the loaded scripts, the scripts loaded after InitWithCallback will be lost.

And I think we shouldn't load pending scripts unconditionally because the child side has changed, the frame scripts needed might be completely different.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Why shouldn't we load pending scripts? That is the whole idea with pending scripts that all
TabChildGlobals will get those scripts loaded.
And for frame mm we probably should put all pending scripts to mPendingScripts/mPendingScriptsGlobalStates, not only in case we don't have mCallback.

If we don't load any scripts, how can the new TabChildGlobal behave as if it was a child side of browser-element?
(though, we do handle browser-elements in rather unique way)

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Why shouldn't we load pending scripts? That is the whole idea with pending
> scripts that all TabChildGlobals will get those scripts loaded.

In that case I agree we should load pending scripts but frame mm is different.

> And for frame mm we probably should put all pending scripts to
> mPendingScripts/mPendingScriptsGlobalStates, not only in case we don't have
> mCallback.

But some scripts are one-off. They didn't expect to be loaded twice. For example I've seen a script that it's sole purpose is to ping back parent.

> If we don't load any scripts, how can the new TabChildGlobal behave as if it
> was a child side of browser-element?
> (though, we do handle browser-elements in rather unique way)

They observe message-manager-changed notification as they observe remote-browser-shown notification and call LoadFrameScripts if needed.

I think if we want to reload all pending scripts, we have to distinguish LoadFrameScriptOneShot and LoadFrameScript. Only latter should get automatically reload.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, perhaps. Though I still would expect pending scripts to be loaded in new TabChildGlobal. Otherwise it may not have the right message listeners which the parent side expects it to have.

billm may have opinion here.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

On desktop, at least, we almost always load frame scripts via the window or "browser" group message manager. If that stuff wasn't loaded in the new TabChild, it would be very strange. So I think pending frame scripts should be loaded in the new child. One-off scripts should be loaded with the aAllowDelayedLoad parameter set to false, so they won't show up in the pending list.

I do wonder if calling loadFrameScript on a frame message manager (as opposed to global or window manager) with aAllowDelayedLoad set to true should even be allowed. In theory it's supposed to allow you to load scripts before everything is initialized. I'm skeptical that we ever actually use that behavior. Even if we do, I think it should be implemented in a different way. JS developers shouldn't have to worry about stuff like that.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd30aa3ebf91

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Always cache pending scripts if delayedLoad is true

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Don't recreate message-manager; force load pending scripts

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a2c4215b8995
https://hg.mozilla.org/integration/b2g-inbound/rev/772f85540b4d

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/a2c4215b8995
https://hg.mozilla.org/mozilla-central/rev/772f85540b4d