Closed
Bug 1186843
Opened 10 years ago
Closed 10 years ago
Notify observers "message-manager-will-change" before switching processes
Categories
(Core :: DOM: Content Processes, defect, P2)
Core
DOM: Content Processes
Tracking
()
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: kanru, Assigned: kanru)
References
Details
Attachments
(2 files, 1 obsolete file)
1.48 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
7.39 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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".
Updated•10 years ago
|
Priority: -- → P1
Updated•10 years ago
|
blocking-b2g: --- → 2.5+
Comment 1•10 years ago
|
||
Hi Kan-Ru,
We are doing triage for unassigned 2.5+ bugs.
Would you take this bug?
Flags: needinfo?(kchen)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kchen
Flags: needinfo?(kchen)
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → FxOS-S5 (21Aug)
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Priority: P1 → P2
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Looks like this is sufficient for now.
Attachment #8658243 -
Flags: review?(bugs)
Comment 6•10 years ago
|
||
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•10 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.
Comment 8•10 years ago
|
||
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•10 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.
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8663506 -
Flags: review?(bugs)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8658243 -
Attachment is obsolete: true
Attachment #8663508 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8663506 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8663508 -
Flags: review?(bugs) → review+
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a2c4215b8995
https://hg.mozilla.org/mozilla-central/rev/772f85540b4d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
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.
Description
•