Closed Bug 757137 Opened 12 years ago Closed 12 years ago

Gaia system app is not a app for the purposes of mozLockOrientation()

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: djf, Assigned: cjones)

References

Details

Attachments

(5 files, 8 obsolete files)

part 1: Force mozbrowser frames to be typeContent (whether or not any other mozbrowser features are enabled)
1.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
part 2: Test
3.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
part 3: Allow system-principal-privileged code to know that a frame really is mozbrowser
1.21 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
part 4: Keep the chrome document on the event target chain when mozbrowser is in a chrome document
1.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part 5.1: Followup fixes for shell.js
11.44 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
The Gaia system app can't call mozLockOrientation, apparently because it is not an <iframe mozapp>, but is instead created by a XUL <browser> tag. Vivien suggests changing b2g/chrome/content/shell.xul to use <html:iframe> instead of <browser>
Attached patch broken work in progress patch (obsolete) — — Splinter Review
I've attached a patch to shell.xul, shell.js and webapi.js that converts the <browser> into an <html:iframe> It doesn't yet work right, though. Issues: 1) The frame is displaying a border. Needs to be turned off. This is probably easy. 2) The back button and the Home button both cause this error: JavaScript Error: "Error: Permission denied for <http://system.gaiamobile.org> to call method ChromeWindow.postMessage" {file: "http://system.gaiamobile.org/js/windows/window_manager.js" line: 231} I assume this is a permission issue, but I don't know what is needed. 3) When the phone puts itself to sleep, there is a sizemodechange event, and the code to handle it tries to access the docShell. But apparently, the <html:iframe> doesn't have one. The code is commented out for now, but presumably we need something to handle this event, and I've got no idea. I'm over my head and am not going to be able to make this work, so I'm assigning it to Vivian.
Assignee: nobody → 21
Would it be better to just leave shell.xul alone, and change mozLockOrientation() to be permission based? We could grant the permission automatically to all installed apps, and then there could be a way for websites that were not full-screen to request permission to lock orientation. It might be a simple doorhanger permission request like geolocation, e.g.
Attached patch Patch (obsolete) — — Splinter Review
This patch make the main app a real app and so mozLockOrientation works! (the pref in the git repository still need to be removed).
Attachment #625798 - Attachment is obsolete: true
Attachment #628716 - Flags: review?(jones.chris.g)
This patch just landed and probably conflicts with your changes to shell.xul: https://hg.mozilla.org/mozilla-central/rev/1861a6f27d8a I'm guessing that your patch will no longer apply cleanly. In particular, note this line: https://hg.mozilla.org/mozilla-central/rev/1861a6f27d8a#l1.18 It assumes that the <browser> element in shell.xul has id "homescreen", so you'll need to patch the new file screen.js to match your change to shell.xul
Comment on attachment 628716 [details] [diff] [review] Patch >diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js >+function getContentWindow() { >+ return content.document.getElementById('system').contentWindow; >+} I'm not sure how this works, but I trust your XUL wizardry ;). > get homeURL() { >+ return Services.prefs.getCharPref('browser.homescreenURL'); >+ }, > >+ get manifestURL() { >+ return Services.prefs.getCharPref('browser.manifestURL'); Design nit: can't we get the URL from the manifest? I would prefer to have one pref for this than two. But we can do that in a followup. I think the term "homescreen" might be a little confusing now in this context, in Gaia, but that's fine for a followup too. One reason for keeping this impl for now is that it'll likely speed up load time a bit compared to reading URL from manifest. If this becomes a problem we need solve for launching apps too, I would prefer to use that solution. >+ let manifestURL = this.manifestURL; >+ let dataURL = >+ 'data:text/html;charset=utf-8,<!DOCTYPE html>' + >+ '<html style="height: 100%; overflow: hidden; padding: 0">' + >+ '<body style="height: 100%; overflow: hidden; margin: 0">' + >+ '<iframe id="system" style="width: 100%; height: 100%; border: 0" ' + >+ 'src="' + homeURL + '" mozallowfullscreen mozbrowser ' + >+ 'mozapp="' + manifestURL + '">'; >+ Is there a reason we can't replace the XUL browser with an html:iframe that directly loads the "system" URL with this styling applied? >+ shell.sendEvent(getContentWindow(), 'mozChromeEvent', { >+ 'type': 'desktop-notification', >+ 'id': id, >+ 'icon': imageUrl, >+ 'title': title, >+ 'text': text >+ }); Please prefer separate patches for formatting fixes. Would like an answer on feasibility of not having iframe-within-browser, since that's not a common case and might affect some platform heuristics we use for gfx.
Attachment #628716 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > >diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js > > Is there a reason we can't replace the XUL browser with an html:iframe > that directly loads the "system" URL with this styling applied? > > > Would like an answer on feasibility of not having > iframe-within-browser, since that's not a common case and might affect > some platform heuristics we use for gfx. That was my first approach but then the Chrome scope leak in Gaia and that why I end up with this approach :/
I don't know what that means.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > I don't know what that means. If I use an html:iframe directly, sub-documents are considered nsIDOMChromeWindow instead of regular nsIDOMWindow because they inherit the scope of shell.xul. This separation is normally done by setting type="content-primary" on the <xul:browser> element.
OK, I understand. Thanks. 1. Can we attach a manifest to <browser content-primary>? 2. Does <html:iframe type="content-primary"> not inherit the chrome scope?
lol, this is hilarious. Our top-level system app runs with no privilege! I agree that we should, somehow, put it inside a mozapp if we can, just because all of our privilege work is based around the notion of mozapp iframes.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > OK, I understand. Thanks. > > 1. Can we attach a manifest to <browser content-primary>? I don't know enough the internal of the <browser> tag to say yes or no but I think BrowserElementChild/BrowserElementParent are not called for it - and so the manifest has no meaning. > 2. Does <html:iframe type="content-primary"> not inherit the chrome scope? 'type' does not mean anything for an iframe.
>> 1. Can we attach a manifest to <browser content-primary>? > > I don't know enough the internal of the <browser> tag to say yes or no but I think > BrowserElementChild/BrowserElementParent are not called for it - and so the manifest has no meaning. I'm pretty sure that you've correctly described the current behavior. We can change this, though. To be clear, the problem is that the system app is written in chrome JS, so it can't be in a vanilla <iframe mozbrowser>?
(In reply to Justin Lebar [:jlebar] from comment #12) > >> 1. Can we attach a manifest to <browser content-primary>? > > > > I don't know enough the internal of the <browser> tag to say yes or no but I think > > BrowserElementChild/BrowserElementParent are not called for it - and so the manifest has no meaning. > > I'm pretty sure that you've correctly described the current behavior. We > can change this, though. > > To be clear, the problem is that the system app is written in chrome JS, so > it can't be in a vanilla <iframe mozbrowser>? The system app is a regular application with no special privileges. It can (should!) be a vanilla <iframe mozbrowser> but sadly I can't (AFAIK) do that from the main xul page because in this case the Chrome scope leaks in the sub-documents tree (and so windows are ChromeWindows). To prevent the Chrome scope to leak the usual way is to use a <browser type="content-primary">. Basically the proposed patch: - use a <browser type="content-primary"> to trace a line between the chrome and the content - load a data:text/html page inside the <browser> element - load a <iframe mozbrowser mozapp> inside the data:text/html page with the system app inside The hierarchy is then: -shell.xul - <browser type="content-primary src="data:text/html,..."> - <iframe mozbrowser mozapp="manifest.webapp"> - <iframe mozbrowser mozapp="manifest.application1.webapp"> - <iframe mozbrowser mozapp="manifest.application2.webapp"> - <iframe mozbrowser mozapp="manifest.application3.webapp"> Before it was: -shell.xul - <browser type="content-primary src="system.html"> - <iframe mozbrowser mozapp="manifest.application1.webapp"> - <iframe mozbrowser mozapp="manifest.application2.webapp"> - <iframe mozbrowser mozapp="manifest.application3.webapp">
(In reply to Vivien Nicolas (:vingtetun) from comment #8) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > > I don't know what that means. > > If I use an html:iframe directly, sub-documents are considered > nsIDOMChromeWindow instead of regular nsIDOMWindow because they inherit the > scope of shell.xul. This separation is normally done by setting > type="content-primary" on the <xul:browser> element. Can anyone argue that this is reasonable behavior for an html:iframe embedded in XUL, particularly as it appears the html:iframe *can't* be prevented from inheriting the XUL scope? Giving html:iframe chrome privs by default without a way to disable just seems like recipe for disaster to me.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #14) > (In reply to Vivien Nicolas (:vingtetun) from comment #8) > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > > > I don't know what that means. > > > > If I use an html:iframe directly, sub-documents are considered > > nsIDOMChromeWindow instead of regular nsIDOMWindow because they inherit the > > scope of shell.xul. This separation is normally done by setting > > type="content-primary" on the <xul:browser> element. > > Can anyone argue that this is reasonable behavior for an html:iframe > embedded in XUL, particularly as it appears the html:iframe *can't* be > prevented from inheriting the XUL scope? > > Giving html:iframe chrome privs by default without a way to disable just > seems like recipe for disaster to me. This belongs to a different issue, isn't it? What about taking proposed approach and change it it we end up with a better fix later?
Not necessarily. If we make html:iframe not inherit the chrome scope, we don't have a problem here right?
bz tells me that this is not good, but we're pretty much stuck for compat reasons.
I think we should fix <browser content-primary>, because this is going to come back and bite us when we want to load "mozapp" in XUL UIs (like FF).
Or alternatively, enable html:iframe to opt-in to not inheriting its docshell type.
This seems to work for me --- the gaia "system app" claims to have a Window, and we fire the mozbrowser-created notification for the html:iframe. bz, I want to see if this approach is totally wrong before writing a real test and requesting review. The platform patch is just a one-liner, at the end.
Attachment #633418 - Flags: feedback?(bzbarsky)
Same nsFrameLoader hunk as before, but puts mozallowfullscreen on the html:iframe. Now fullscreen works too!
Attachment #633418 - Attachment is obsolete: true
Attachment #633418 - Flags: feedback?(bzbarsky)
Attachment #633430 - Flags: feedback?(bzbarsky)
I get [JavaScript Error: "TypeError: this.contentBrowser.docShell is undefined" {file: "chrome://browser/content/shell.js" line: 271}] with this patch, so def. needs more work.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22) > I get [JavaScript Error: "TypeError: this.contentBrowser.docShell is > undefined" {file: "chrome://browser/content/shell.js" line: 271}] with this > patch, so def. needs more work. Do you have OOP enabled? That could cause this...
The problem is that we're asking for the docShell of an html:iframe, which doesn't have that getter. We need some QI gymnastics to get at its docshell.
Comment on attachment 633430 [details] [diff] [review] Let chrome-type non-XUL frames opt-in to typeContent, v2 The problem is I expect that there are extensions cargo-culting the type="content" on HTML iframes but relying on it not working. Can we restrict looking at the type="content" bit for HTML iframes to the case when mozbrowser is also used? Other than that, I think this is fine.
Attachment #633430 - Flags: feedback?(bzbarsky) → feedback+
/me goes to take a shower
Assignee: 21 → jones.chris.g
Attachment #628716 - Attachment is obsolete: true
Attached patch part 2: Test — — Splinter Review
A bit out of my element here, want to ensure I cargo-culted everything correctly.
Attachment #633736 - Flags: review?(bzbarsky)
This is the manifest changes pirated from your earlier patch.
Attachment #633737 - Flags: review?(21)
Comment on attachment 633734 [details] [diff] [review] part 1: Force mozbrowser frames to be typeContent (whether or not any other mozbrowser features are enabled) r=me
Attachment #633734 - Flags: review?(bzbarsky) → review+
Comment on attachment 633736 [details] [diff] [review] part 2: Test s/haves/behaves/ s/"_new"/""/ It's worth testing that otherWindow _does_ QI to nsIDOMChromeWindow. r=me assuming the test passes (as in, you get otherWindow before its load event has fired).
Attachment #633736 - Flags: review?(bzbarsky) → review+
Comment on attachment 633737 [details] [diff] [review] part 3: Update b2g "chrome" to use html:iframe mozbrowser, and set a manifest for the system app Review of attachment 633737 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.xul @@ +23,5 @@ > <script type="application/javascript" src="chrome://browser/content/screen.js"/> > #endif > + <html:iframe id="homescreen" > + mozbrowser="true" mozallowfullscreen="true" > + style="overflow: hidden; width: 320px; height: 480px;" add -moz-box-flex: 1; to get resizing working!
Attachment #633737 - Flags: review?(21) → review+
(In reply to Boris Zbarsky (:bz) from comment #31) > Comment on attachment 633736 [details] [diff] [review] > part 2: Test > > s/haves/behaves/ > > s/"_new"/""/ > Done. > It's worth testing that otherWindow _does_ QI to nsIDOMChromeWindow. > Done. > r=me assuming the test passes (as in, you get otherWindow before its load > event has fired). I don't understand your concern 100%, but the test fails before part 1 and passes after.
(In reply to Fabrice Desré [:fabrice] from comment #32) > Comment on attachment 633737 [details] [diff] [review] > part 3: Update b2g "chrome" to use html:iframe mozbrowser, and set a > manifest for the system app > > Review of attachment 633737 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/chrome/content/shell.xul > @@ +23,5 @@ > > <script type="application/javascript" src="chrome://browser/content/screen.js"/> > > #endif > > + <html:iframe id="homescreen" > > + mozbrowser="true" mozallowfullscreen="true" > > + style="overflow: hidden; width: 320px; height: 480px;" > > add -moz-box-flex: 1; to get resizing working! Worked like a charm. Also allowed me to remove the crud from shell.js Aaaaand I noticed that the dorky default iframe border was appearing on my phone screen. Reminds me of the "[Object object]" network operator that philikon saw at MWC. These shall hereafter be known as "B2G Moments". (border: none took care of the html:iframe.)
I'm running into a problem now after rebasing that I didn't see before, which is that we're no longer firing the mozbrowser-created notification. It's coming down to nsIPrincipal *principal = NodePrincipal(); nsCOMPtr<nsIURI> principalURI; principal->GetURI(getter_AddRefs(principalURI)); if (!nsContentUtils::URIIsChromeOrInPref(principalURI, "dom.mozBrowserFramesWhitelist")) { NodePrincipal() gives us the SystemPrincipal, which has a null principalURI and we bail in URIIsChromeOrInPref(). My understanding is that the system principal should get a free ride here.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #35) > My understanding is that the system > principal should get a free ride here. You can make an nsContentUtils::PrincipalIsChromeOrInPref which does the check for system principal and then passes on to URIIsChromeOrInPref, I guess. All of this is going to change when we have the much-vaunted Permissions Manager.
Drat, had wanted to land this tonight, but I don't see how things could have worked for me without this patch.
Attachment #633769 - Flags: review?(justin.lebar+bug)
This ended up changing enough that re-review is probably called for. On the bright side, much cleaner now!
Attachment #633737 - Attachment is obsolete: true
Attachment #633770 - Flags: review?(fabrice)
(In reply to Justin Lebar [:jlebar] from comment #36) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #35) > > My understanding is that the system > > principal should get a free ride here. > > You can make an nsContentUtils::PrincipalIsChromeOrInPref which does the > check for system principal and then passes on to URIIsChromeOrInPref, I > guess. > > All of this is going to change when we have the much-vaunted Permissions > Manager. I had patched and was off to the races before I saw your comment. Let me know what you prefer.
Attachment #633770 - Flags: review?(fabrice) → review+
Attached patch Followup fixes (obsolete) — — Splinter Review
While working on something else, I saw a bunch of stuff was broken by the earlier changes, specifically no content-primary frame :/. Yay for magical globals. This fixes the easy stuff. For some reason, the HOME key is not delivered to shell.js at all. All the other HW keys are still delivered. Short-pressing HOME doesn't do anything, but long-pressing it still works. Will need to resume later.
The new screenshooter seems to be still ~working (vibrates when I do home+power, but no screenshots), so this looks like maybe an issue with event capture.
Fabrice helpfully pointed me at the BrowserElementChild.js filtering code, which seems to be coming into play here. I'm testing on desktop but the behavior seems identical on phone. The HOME key is *not* in the whitelist propagated out of content. Here are some behaviors I'm seeing Lockscreen shown, just after window opens: [shell.js] KEY keydown: 36, capturing? true (stopping propagation) !!WTF!! [screenshot.js] KEY DOWN: 36 !!WTF!! [window_manager.js] KEY DOWN: 36 [BrowserElementChild.js] KEY keydown: 36, capturing? true Click lockscreen: !!WTF!! [screenshot.js] KEY DOWN: 36 [window_manager.js] KEY DOWN: 36 [BrowserElementChild.js] KEY keydown: 36, capturing? true Homescreen shown: !!WTF!! [BrowserElementChild.js] KEY keydown: 36, capturing? true [BrowserElementChild.js] KEY keydown: 36, capturing? true App shown: !!WTF!! [BrowserElementChild.js] KEY keydown: 36, capturing? true [BrowserElementChild.js] KEY keydown: 36, capturing? true After b2g first launches, shell.js is getting events on its <window>. HOWEVER, it can't stop propagation of them :(. When I click the lockscreen (presumably focusing it), shell.js stops receiving events on <window>. I have no idea what's going on here. I'm going to try a hack then I'll need to move on to something else until I can get backup from folks who know what they're doing.
Event handling is going to be a security/DoS issue, so we need to nail this down hard.
Wrapping the html:iframe in a XUL <box> and attaching event listeners to the <box> doesn't work around the problem either :( (unsurprisingly).
Alright, a further patch that enables <html:iframe mozbrowser> to declare type="content-primary" doesn't work either. Same behavior as in comment 42.
I confirmed that shell.js is capturing the events without these patches (i.e. that this wasn't working by accident previously). So in summary - with <window><browser>, event listeners that shell.js attach to the <window> *are* fired. - with <window><html:iframe>, the <window> event listeners *don't* fire. - with <window><html:iframe type="content-primary"> (patched to actually work), the <window> event listeners don't fire either. This seems like a bug to me.
Comment on attachment 633769 [details] [diff] [review] part 3: Allow system-principal-privileged code to know that a frame really is mozbrowser r=me, but we really should fix the nsContentUtils method; all usages of URIIsChromeOrInPref get a URI off a principal, and have this same bug. I'll file a bug on that change; it doesn't need to block you here.
Attachment #633769 - Flags: review?(justin.lebar+bug) → review+
<smaug> cjones: Components.classes["@mozilla.org/eventlistenerservice;1"].getService(Components.interfaces.nsIEventListenerService).getEventTargetChainFor(youreventtarget); <smaug> that gives you an array of event targets * Jesse (jruderman@moz-BBE3ABD.mv.mozilla.com) has joined #content <cjones> and |youreventtarget| is the HTML element? or |window|? <smaug> in your case you're interested in the window <smaug> iframe.contentWindow
Well! With the latest patches, for some reason window.addEventListener('ContentStart', function onload(e) { is no longer firing! That is not expected.
This is what things look like when HOME works properly: --- AFTER STARTUP --- Event target chain: [object Window @ 0x7f8063a45fd0 (native @ 0x7f8063cb3c80)],[object ContentFrameMessageManager @ 0x7f8063c78190 (native @ 0x7f8063c6c740)],[xpconnect wrapped nsIDOMEventTarget @ 0x7f8063b89b30 (native @ 0x7f80655fb940)] and here too: --- INTERVAL TIMER --- Event target chain: [object Window @ 0x7f8063ae2580 (native @ 0x7f8063bd7880)],[object ContentFrameMessageManager @ 0x7f8063c78190 (native @ 0x7f8063c6c740)],[xpconnect wrapped nsIDOMEventTarget @ 0x7f8063b89b30 (native @ 0x7f80655fb940)] This is what things look like when HOME does *not* work properly: --- INTERVAL TIMER --- Event target chain: [object Window @ 0x7f8063ae2580 (native @ 0x7f8063bd7880)],[object ContentFrameMessageManager @ 0x7f8063c78190 (native @ 0x7f8063c6c740)],[xpconnect wrapped nsIDOMEventTarget @ 0x7f805ede2e80 (native @ 0x7f80655fb940)] I don't really know where to go from here.
When the event dispatch works properly, I see Breakpoint 2, nsWindow::OnKeyPressEvent (this=0x7ffff6c65680, aWidget=0x7fffd547d080, aEvent=0x7fffcdbc10f0) at /home/cjones/mozilla/mozilla-central/widget/gtk2/nsWindow.cpp:3033 $139 = (nsChildWindow *) 0x7ffff6c65680 $140 = (EVENT_CALLBACK) 0x7ffff2329b70 <HandleEvent(nsGUIEvent*)> and when it doesn't, I see Breakpoint 2, nsWindow::OnKeyPressEvent (this=0x7ffff6c65680, aWidget=0x7fffd547d080, aEvent=0x7fffceb32120) at /home/cjones/mozilla/mozilla-central/widget/gtk2/nsWindow.cpp:3033 $141 = (nsChildWindow *) 0x7ffff6c65680 $142 = (EVENT_CALLBACK) 0x7ffff2329b70 <HandleEvent(nsGUIEvent*)> so everything is the same as far as widgetry is concerned. The problem must lie in DOM land.
So to summarize - I see the same event target chain for working/not-working cases - I see the same widget and event handler for working/not-working cases I'm more than happy to poke around some more, but I don't know where else to look. Halp! :)
Attached patch Followup fixes, v2 (+logging) (obsolete) — — Splinter Review
Attachment #633784 - Attachment is obsolete: true
<smaug> from message manager event dispatching goes usually to the element which owns it <smaug> so, xul:browser <smaug> and from xul:browser to xul:window (or other parentNode) <smaug> then to |window| <smaug> and then WindowRoot <smaug> in this case nsInProcessTabChildGlobal::PreHandleEvent actively bypasses xul:browser/xul:window/|window| <smaug> if (mIsBrowserFrame) { -> if (mIsBrowserFrame && !nsContentUtils::IsInChromeDoc(mOwner))) { <smaug> that should fix this <smaug> er, what is the right contentutils method <cjones> i will gladly admit that i don't fully understand, but i'm happy to give that a try :) <smaug> if (mIsBrowserFrame && (!mOwner || !nsContentUtils::IsInChromeDocshell(mOwner->OwnerDoc()))
Comment on attachment 634580 [details] [diff] [review] part 5.1: Followup fixes for shell.js Review of attachment 634580 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +228,5 @@ > let rootContentEvt = (evt.target.ownerDocument.defaultView == content); > if (!rootContentEvt && evt.eventPhase == evt.CAPTURING_PHASE && > evt.keyCode == evt.DOM_VK_HOME) { > + > + dump(" (stopping propagation)\n"); leftover?
Attachment #634580 - Flags: review?(fabrice) → review+
Yes, I had already removed it locally. Thanks for the steal.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: