Closed Bug 725796 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fabrice, Assigned: justin.lebar+bug)

References

()

Details

Attachments

(2 files, 4 obsolete files)

When loading http://flickr.com in the b2g browser, they claim that "flickr doesn't allow embedding within frames".
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.
Summary: Some sites don't want to be loaded in iframes → Try vingtetun's window.top hack
Here the ugly hack. I can load flickr.com with it - only tried on my laptop.
Assignee: nobody → 21
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.
Unfortunately this patch doesn't solve the problem for http://flickr.com
(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.
I tested with other sites (on device):
http://touch.facebook.com/ and http://m.yelp.com load without problems.
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?
> 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.
Well, mozbrowser or mozapp.  The machinery in gecko will be similar, but the interface might be different.
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?
(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?
(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...)
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.
But, I think we're discussion something that's not relevant to this bug :).  Sorry for sidetracking.
We should land this if this works.
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.
Attached patch patch (obsolete) — Splinter Review
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...
Attachment #597918 - Flags: review?(jones.chris.g)
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.
Attachment #597918 - Flags: review?(jones.chris.g)
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?
I wonder if DOMWindowCreated will do the job here. See bug 549539.
I think it should, yes.

You probably need to also set window.parent to window and window.frameElement to null, right?
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.
Summary: Try vingtetun's window.top hack → Make <iframe mozbrowser> a window.{top,parent,frameElement} barrier to script
Attached patch Patch v1 (obsolete) — Splinter Review
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.  :)
Attachment #599152 - Flags: review?(bzbarsky)
Assignee: 21 → justin.lebar+bug
Attachment #597918 - Attachment is obsolete: true
Attachment #596009 - Attachment is obsolete: true
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...
Attachment #599152 - Flags: review?(bzbarsky) → review-
> 2)  Probably need to make sure that frameloader swaps don't swap across different
>     mozbrowserness, right?

Can I write a test for this?
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.
Attached patch Patch v2 (obsolete) — Splinter Review
I didn't end up writing a test for the frameloader swap -- the code is pretty straightforward.
Attachment #599152 - Attachment is obsolete: true
Attachment #602063 - Flags: review?(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!
Attachment #602063 - Flags: review?(bzbarsky) → review+
> Those are already in scope.  How did this compile?

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

Go me!  ;)
Attached patch Patch v3Splinter Review
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.)
Attachment #602063 - Attachment is obsolete: true
Attachment #602474 - Flags: review?(bzbarsky)
Comment on attachment 602474 [details] [diff] [review]
Patch v3

r=me
Attachment #602474 - Flags: review?(bzbarsky) → review+
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/jlebar@mozilla.com-5bf108f35fba
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.
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/jlebar@mozilla.com-2253a8bb05a5
 Timed out after 06 hours without completing.
https://hg.mozilla.org/mozilla-central/rev/ab7ed8a7a885
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.