584864 - MessageManager's LoadFrameScript runs in Chrome windows

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

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.

Comment 1

[Alon Zakai (:azakai)]

Attachment: Patch

Patch to restore the code that made this work ok (chrome windows are not called), that was lost in a merge at some point.

Comment 2

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

Comment on attachment 463334 [details] [diff] [review]
Patch

I'll land this tomorrow.

Comment 3

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

...unless someone lands it before :)

Comment 4

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

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.

Comment 5

[Alon Zakai (:azakai)]

This is needed for bug 550936, which is blocking2.0 beta4+ and blocking-fennec 2.0a1+.

Comment 6

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

I'll push this once my build is ready.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/dea6175f4b07

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch breaks loading IPC web pages in Fennec (see bug 585606)

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out to fix the regression:
http://hg.mozilla.org/mozilla-central/rev/25a4b29f243b

Comment 10

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

Uh.

We really need some IPC mochitests.

The fix should be. 
if (!mIsTopLevelContent && !mRemoteFrame) {
  return;
}
Could someone with e10s-fennec verify that it works.

Comment 11

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

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.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

New patch. Tested and confirmed that it works for Fennec.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/6820ff3f2027

Comment 15

[Nomis101]

/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

Comment 16

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

Uh, we need ifdef MOZ_IPC.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 18

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

The fix for this landed already, I think.

Comment 19

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

http://hg.mozilla.org/mozilla-central/rev/e5a5061b99d1

Hopefully it works