725796 - Make <iframe mozbrowser> a window.{top,parent,frameElement} barrier to script

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

When loading http://flickr.com in the b2g browser, they claim that "flickr doesn't allow embedding within frames".

Comment 1

[Justin Lebar (not reading bugmail)]

Specifically, bug 714861 is addressing this issue.  But vingtetun has said he has a chrome-JS hack, which I'd really like to see in action.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Rewrite window.top to point to the same browser window

Here the ugly hack. I can load flickr.com with it - only tried on my laptop.

Comment 3

[Justin Lebar (not reading bugmail)]

I think we should check this in and see how it works with other sites.

Getting cross-process mozbrowser working in two weeks is going to be a stretch.

Comment 4

[[:fabrice] Fabrice Desré]

Unfortunately this patch doesn't solve the problem for http://flickr.com

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Unfortunately this patch doesn't solve the problem for http://flickr.com

Device or desktop? Works for me on desktop. It could be a race if it doesn't work on the device.

Comment 6

[[:fabrice] Fabrice Desré]

I tested with other sites (on device):
http://touch.facebook.com/ and http://m.yelp.com load without problems.

Comment 7

[Ben Francis [:benfrancis]]

That's fantastic!

I think we need to do this for app iframes too, at least for the social networking app for now.

One way to do this might be to apply the window.top hack to all iframes with the "mozbrowser" attribute rather than by id, then add the mozbrowser attribute to all app iframes as well?

Comment 8

[Justin Lebar (not reading bugmail)]

> One way to do this might be to apply the window.top hack to all iframes with the "mozbrowser" 
> attribute rather than by id, then add the mozbrowser attribute to all app iframes as well?

Yes, please.  App iframes are going to have to be mozbrowser anyway, to get the isolation we want.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Well, mozbrowser or mozapp.  The machinery in gecko will be similar, but the interface might be different.

Comment 10

[Justin Lebar (not reading bugmail)]

There's no immediate need for them to be different that I'm aware of, so while we're doing a hack, can we just mozbrowser on all the app iframes?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #10)
> There's no immediate need for them to be different that I'm aware of, so
> while we're doing a hack, can we just mozbrowser on all the app iframes?

Sure.
I don't think there is any other iframes in Gaia right now but I can make the hack more flexible. Or maybe ben's want to take it?

Comment 12

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Well, mozbrowser or mozapp.  The machinery in gecko will be similar, but the
> interface might be different.

Also, we wouldn't be able to do this hack on a proper mozapp iframe, because it wouldn't fire locationchange.  (At least, I don't think we'd need to keep track of the location of an app iframe...)

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Everything that the browser app is notified off from its <iframe mozbrowser>, we *don't* want a "window manager" notified of for <iframe mozapp>.  mozapp is a black box.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

But, I think we're discussion something that's not relevant to this bug :).  Sorry for sidetracking.

Comment 15

[Andreas Gal :gal]

We should land this if this works.

Comment 16

[Ben Francis [:benfrancis]]

I'm just trying to modify this patch so that it applies to all iframes with the mozbrowser attribute, but we have to be careful how we do it because of race conditions. The current patch seems to (mostly) work for the browser app, but we also need this for the social networking and game apps which requires a slightly different solution.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch

This one works on device for me with an app pointing to touch.facebook.com.
It's even uglier than the previous one but it should enough. I'm a little ashamed...

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 597918 [details] [diff] [review]
patch

>+          // Overrride application's window.top
>+          let interval = content.setInterval(function overrideWindowInterval() {
>+            overrideWindowTop(topWindow);
>+          }, 1);
>+

This is going to kill perf.  Do you really need to do this "forever"?
Are you trying to avoid racing with Gecko setting up the "real"
window.top?  There's probably some event we can listen to in chrome
that correlates with that.  I would sadly r+ if we did this for a
second or two.  But hopefully bz can save us.

CC'ing bz.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bz, see comment 18: we're not going to have a complete non-hacky solution for creating a window.top boundary before MWC, so we're looking for an effective hacky substitute.  Let's assume that no hacky is too hacky for now.

Is there some event that correlates well with when gecko sets up window.top, which would be a good time for us to set up the hack here?  Or do you have other suggestions for a temporary hack here?

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I wonder if DOMWindowCreated will do the job here. See bug 549539.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I think it should, yes.

You probably need to also set window.parent to window and window.frameElement to null, right?

Comment 22

[Justin Lebar (not reading bugmail)]

I have a hacky C++ solution, which makes window.top and friends respect mozbrowser only when called by script.

I'll post it in a few minutes.

Comment 23

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

FYI, I'm going to check this into b2g's m-c clone as-is, since people are waiting on this for our demo.  Hopefully it's not too busted.  :)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 599152 [details] [diff] [review]
Patch v1

So there are two correctness issues here:

1)  Need to rev iids on the IDL files you changed.
2)  Probably need to make sure that frameloader swaps don't swap across different
    mozbrowserness, right?

In addition to those, how does the performance of the new window.top compare to the old one from script?  I guess they're probably both slow...

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, another follow-up build fix landed on cgjones/mozilla-central: https://github.com/cgjones/mozilla-central/commit/74302cbf5e5b7953412f851128203b668356efad

Comment 26

[Justin Lebar (not reading bugmail)]

> 2)  Probably need to make sure that frameloader swaps don't swap across different
>     mozbrowserness, right?

Can I write a test for this?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Sure.  It'd need to be a chrome test, but other than that, should be easy enough.  http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#1199 is an example of invoking the swap.

Comment 28

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

I didn't end up writing a test for the frameloader swap -- the code is pretty straightforward.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 602063 [details] [diff] [review]
Patch v2

>+++ b/content/base/src/nsFrameLoader.cpp
>+  nsCOMPtr<nsIDocShell> ourDocShell = do_QueryInterface(ourTreeItem);
>+  nsCOMPtr<nsIDocShell> otherDocShell = do_QueryInterface(otherTreeItem);

Those are already in scope.  How did this compile?

>+  if (!ourDocShell || !otherDocShell) {
>+    return NS_ERROR_NOT_IMPLEMENTED;

And this has already been checked.

The rest looks fine, but I'd really like to understand what's going on with our/otherDocShell!

Comment 31

[Justin Lebar (not reading bugmail)]

> Those are already in scope.  How did this compile?

Ah, because they're called "ourDochell" (sic) and "otherDocshell".

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Ah, because they're called "ourDochell" (sic) and "otherDocshell".

Go me!  ;)

Comment 33

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

GetParent can return null, and it looks like doing a->GetParent(getter_AddRefs(a)) was causing badness.

(Sorry to ask for another review; I thought I'd tested this patch, but I was wrong.)

Comment 34

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v2 --> v3

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 602474 [details] [diff] [review]
Patch v3

r=me

Comment 36

[Mozilla RelEng Bot]

Try run for 5bf108f35fba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5bf108f35fba
Results (out of 14 total builds):
    exception: 5
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5bf108f35fba

Comment 37

[Justin Lebar (not reading bugmail)]

Good thing I pushed to try; this worked locally but failed when I clobbered, because on clobber dom/base IDL parsing was happening before dom/html.

Comment 38

[Mozilla RelEng Bot]

Try run for 2253a8bb05a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2253a8bb05a5
Results (out of 221 total builds):
    exception: 1
    success: 179
    warnings: 40
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2253a8bb05a5
 Timed out after 06 hours without completing.

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab7ed8a7a885