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

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

unspecified
FxOS-S8 (02Oct)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
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".
Priority: -- → P1
blocking-b2g: --- → 2.5+
Hi Kan-Ru,
We are doing triage for unassigned 2.5+ bugs.
Would you take this bug?
Flags: needinfo?(kchen)
Assignee

Updated

4 years ago
Assignee: nobody → kchen
Flags: needinfo?(kchen)
Assignee

Updated

4 years ago
Duplicate of this bug: 1186296
Assignee

Updated

4 years ago
Target Milestone: --- → FxOS-S5 (21Aug)
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
Flags: needinfo?(kchen)
Assignee

Comment 4

4 years ago
I'm working on it.
Flags: needinfo?(kchen)
Priority: P1 → P2
Status: NEW → ASSIGNED
Assignee

Comment 5

4 years ago
Looks like this is sufficient for now.
Attachment #8658243 - Flags: review?(bugs)
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?
Attachment #8658243 - Flags: review?(bugs) → review-
Assignee

Comment 7

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

Comment 9

4 years ago
(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.
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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Attachment #8663506 - Flags: review?(bugs) → review+
Attachment #8663508 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a2c4215b8995
https://hg.mozilla.org/mozilla-central/rev/772f85540b4d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: FxOS-S5 (21Aug) → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.