Closed Bug 584864 Opened 11 years ago Closed 11 years ago

MessageManager's LoadFrameScript runs in Chrome windows

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
fennec 2.0a1+ ---

People

(Reporter: azakai, Assigned: azakai)

References

Details

Attachments

(1 file, 1 obsolete file)

If LoadFrameScript is called (see e.g. the patch in bug 550936) in Firefox, then since Firefox creates 4 DocShells for the first window, the frame script is actually loaded 4 times - once in the content window, and once for each of the 3 chrome windows.

This leads to noticeable slowdowns (see discussion in bug 550936). In some cases we would only want the script to be loaded in one type of window - content or chrome. So adding an option for the script to only be loaded in one type would be nice.
Blocks: 550936
Summary: Let MessageManager's LoadFrameScript specify Chrome and/or Content → MessageManager's LoadFrameScript runs in Chrome windows
Attached patch Patch (obsolete) — Splinter Review
Patch to restore the code that made this work ok (chrome windows are not called), that was lost in a merge at some point.
Attachment #463334 - Flags: review?(Olli.Pettay)
Comment on attachment 463334 [details] [diff] [review]
Patch

I'll land this tomorrow.
Attachment #463334 - Flags: review?(Olli.Pettay) → review+
...unless someone lands it before :)
Comment on attachment 463334 [details] [diff] [review]
Patch

Oops, approval is needed.

This patch adds back code which was lost in some 
e10s->m-c or m-c->e10s merge.
Attachment #463334 - Flags: approval2.0?
Assignee: nobody → azakai
This is needed for bug 550936, which is blocking2.0 beta4+ and blocking-fennec 2.0a1+.
blocking2.0: --- → ?
tracking-fennec: --- → ?
Attachment #463334 - Flags: approval2.0? → approval2.0+
tracking-fennec: ? → 2.0a1+
I'll push this once my build is ready.
http://hg.mozilla.org/mozilla-central/rev/dea6175f4b07
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
This patch breaks loading IPC web pages in Fennec (see bug 585606)
Backed out to fix the regression:
http://hg.mozilla.org/mozilla-central/rev/25a4b29f243b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Uh.

We really need some IPC mochitests.

The fix should be. 
if (!mIsTopLevelContent && !mRemoteFrame) {
  return;
}
Could someone with e10s-fennec verify that it works.
Or actually the E10s setup in frameloader is just a bit wrong, 
since it doesn't set mIsTopLevelContent.
But comment 10 should be enough still.
(In reply to comment #10)
> Uh.
> 
> We really need some IPC mochitests.
> 
> The fix should be. 
> if (!mIsTopLevelContent && !mRemoteFrame) {
>   return;
> }
> Could someone with e10s-fennec verify that it works.

I tested in e10s-fennec. This does in fact work.
Attached patch patch 2Splinter Review
New patch. Tested and confirmed that it works for Fennec.
Attachment #463334 - Attachment is obsolete: true
Attachment #464081 - Flags: review?(Olli.Pettay)
Attachment #464081 - Flags: review?(Olli.Pettay) → review+
Blocks: 552828
pushed:
http://hg.mozilla.org/mozilla-central/rev/6820ff3f2027
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
/builds/slave/comm-central-trunk-macosx/build/mozilla/content/base/src/nsFrameLoader.cpp:1892: error: 'mRemoteFrame' was not declared in this scope
make[8]: *** [nsFrameLoader.o] Error 1

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1281455252.1281455917.13884.gz
Uh, we need ifdef MOZ_IPC.
Smaug - Do we need MOZ_IPC and non MOZ_IPC cases?

#ifdef MOZ_IPC
 if (!mIsTopLevelContent && !mRemoteFrame) {
   return;
 }
#else
 if (!mIsTopLevelContent) {
   return;
 }
#endif

Like that?
The fix for this landed already, I think.
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.