I feel like this must be a dup, but I can't find it, so here goes: We should rewrite the feed preview functionality to happen in a non-privileged way, because doing otherwise is a continuing source of problems. I know mconnor (cc'd) has thoughts on this too, but in any event I wanted the bug on file. Right now the page "needs" privilege because it: a) gets privileged information (the list of feed readers you have registered) b) does privileged things (subscribes using registered feed readers, "remember this setting", &c) B) is a solved problem, we solved it in SSL error pages and in the malware error pages. We drop page privilege and when our magic buttons get prodded, the events bubble up to chrome, where we catch them, make sure they aren't synthetic and are what we expect, and then process them up in chrome. A) is harder, but not impossible and certainly no need to leave privilege on the page. The code handling the about: page generation already inserts this content into the page, so it should do that and then drop privs which, given our solution to B), should leave us just skippy. If you want to solve it a different way, that's super, but at least there's a bug now. This can't make FF3.1 but it should be the very next thing we do, imo.

(In reply to comment #0) > If you want to solve it a different way, that's super, but at least there's a > bug now. This can't make FF3.1 but it should be the very next thing we do, imo. Where's the blocking-firefox3.2 flag for me to request? ;)

The feed preview page isn't privileged, it handles this sort of issues the same way you did.

I'm moving this bug to the MoCo group since I cannot see a similiar checkbox for the security group, please move it for me. The feed preview security issues are: * The FeedWriter component can be QIed to various interfaces in the content context. To be honest, I have no idea why should content code be able to access anything but ClassInfo-exposed methods (i.e. it shouldn't be able to QI). Anyway, if we cannot fix that (backwards compatibility?), we should move most of the feedwriter functionality into a tear off. * Chrome code (the component) used to manipulate the preview-page DOM, and by that it dispatches various DOM events (attribute change, node added etc.). When there are certain XSS exploits in place, content code was/is able to execute privileged code by listening to these events. We solved this by doing all DOM manipulations (which would result in document-level events) in a content-principal sandbox. A better fix would be to move all DOM-modifications to the feed-preview-page script (which has no chrome privileges). To do so, we need to morph the FeedWriter object to some sort of GiveMeInfoAboutThisFeed object. CCing Boris and Brendan as they might be able to answer my question about QueryInterface in content context.

We might be able to fix the first issue for 3.1 using a tear off. The second issue needs much more work and alpha-baking.

> The FeedWriter component can be QIed to various interfaces in the content > context. To be honest, I have no idea why should content code be able to access > anything but ClassInfo-exposed methods That's actually controllable if your classinfo's getHelperForLanguage for JS returns an nsIXPCScriptable with the CLASSINFO_INTERFACES_ONLY flag set. All of our DOM objects do so, basically. The only catch is that nsIXPCScriptable is not a scriptable interface. That said, I see no reason it couldn't be, with all the methods noscript except for className and scriptableFlags. As long as you don't set the flags asking for any of the hooks to be called, you should be fine... ccing shaver and xpconnect owners/peers, because I know shaver loves nsIXPCScriptable. ;) Even without that change you could just have a tiny C++ nsIXPCScriptable implementation here that does the right thing for you. I think we should reopen this bug and retitle as needed (perhaps "Stop the Feed Preview security whack-a-mole"?).

Moved to security gp, btw.

And actually ccing xpconnect folks.

(In reply to comment #3) > A better fix would be to move all DOM-modifications > to the feed-preview-page script (which has no chrome privileges). To do so, we > need to morph the FeedWriter object to some sort of GiveMeInfoAboutThisFeed > object. That's what I assumed this bug was about, though implemented in a way that put the feed reader info in the page itself, rather than having it have to call in to privileged code.

I'm going to reopen and resummarize, to cover the original intent. bz's plan in comment 5 sounds like a step in the right direction, though not having to call into privileged code at all would still be best, I think.

(In reply to comment #3) > * Chrome code (the component) used to manipulate the preview-page DOM, and by > that it dispatches various DOM events (attribute change, node added etc.). When > there are certain XSS exploits in place, content code was/is able to execute > privileged code by listening to these events. We solved this by doing all DOM > manipulations (which would result in document-level events) in a > content-principal sandbox. Is this still true? It makes the feed preview code very ugly :-(

Is there anyone who can take this bug? I'll agree with Gavin that comment 5 sounds like a reasonable approach.

Was this not fixed by COWs?

I know bug 454363 was fixed by COWs. Do COWs cover us across this broader set of FeedWriter bugs? I don't know enough about them to make that assertion, but if so, we can probably RESOLVE FIXED this.

I suspect COWs mitigate the risk of this page continuing to be chrome privileged, but as a basic principle, I don't think we should have chrome privilege on any page that can be linked to from, and parses, attacker-controlled content. This bug is about replacing the feed preview with something unprivileged, and I still think that's a bug worth fixing. If it were fixed, I wouldn't worry about unknown COW bugs coming up to bite us through this interface. I would totally not object to us lowering its security bug status, though, or maybe even unhiding it? What do you think, dveditz? IIRC this is your pet peeve, too.

I've done some investigation of the current architecture and come up with some possible approaches for making it more secure. Summary of Possible Approaches ------------------------------ * Eliminate RSS preview. If this is done, the RSS subscription can be done entirely in chrome with no threat from Internet content. This seems like it might be a contentious issue, and I have not examined it in depth. * Change the way that the unprivileged about:feeds page communicates with privileged code. I haven't looked at this in detail yet, but it should probably be my next area of focus. * Split feed preview generation into two phases. DOM creation would be deferred to the second phase and that phase could be run entirely inside the unprivileged about:feeds page. This is where most of my focus has been so far. I think this is feasible but might take a couple of days of focused effort. * Separate the subscription UI from the about:feeds page. This is at the "idle speculation" stage right now. Eliminate RSS preview --------------------- Limi and Faaborg agree that it might be possible to eliminate RSS preview, but not RSS subscription. Johnath notes that preview is useful for pages which offer multiple RSS feeds and that any attempt to float a proposal for removing preview in Firefox 3.7 at the same time as we're trying to ship Firefox 3.6 is bound to cause confusion. Change the way that the unprivileged about:feeds page communicates with privileged code --------------------------------------------------------------------------------------- The about:feeds page creates a FeedWriter (AKA BrowserFeedWriter) object to communicate with the privileged JavaScript that handles most of the RSS preview and subscription behavior. The FeedWriter can actually be created by any web page, and a page can pass whatever arguments it wants to the various methods exposed by the FeedWriter object. The FeedWriter implementation has been hardened against these sorts of attacks (see bugs 352124, 352124, and 454363). This seems like an excellent opportunity to employ the "Hollywood Principle" (don't call us, we'll call you). This approach was mentioned and dismissed in passing in bug 352124, but it seems to me like it should be easy to do. The major problem seems to be identifying whether a page is really the about:feeds page (the URI is not "about:feeds" but rather the URI of the RSS feed). This seems like it should be a surmountable problem. This needs more investigation, but it seems like this aspect of the design should be pretty easy to fix. RSS Preview Generation ---------------------- RSS feed parsing in handled by privileged code. This code uses a SAX-based parser which is pretty low-level. I think the parser would be hard-pressed to execute rogue JavaScript in the feed even if it wanted to. Things get interesting when the preview DOM is built, however. The preview DOM is built by privileged JavaScript, but again it's been hardened against possible attacks. Notably it uses a sandbox when inserting DOM nodes into the about:feeds page. A more robust design would split feed preview generation into two phases, with only the second phase responsible for DOM creation. This second phase could then be run in its entirety with reduced privilege inside the about:subscribe page. Mano mentions this kind of approach in a comment on bug 474698. I think it might make sense to pass the information from phase 1 (privileged) to phase 2 (unprivileged) as JSON. I think I could probably do this work in a couple of days (to the first reviewable patch), given minimal interruptions. Separate the subscription UI from the about:feeds page ------------------------------------------------------ The about:feeds page includes chrome controls for subscribing to the feed. Mixing these controls with the otherwise untrusted content in the feed preview provides more avenues for attack. I can think of several ways we could approach this, for example: CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC CssssssssssssssssssssssssssssssssssssssssssC CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC CppppppppppppppppppppppppppppppppppppppppppC CppppppppppppppppppppppppppppppppppppppppppC CppppppppppppppppppppppppppppppppppppppppppC CppppppppppppppppppppppppppppppppppppppppppC CppppppppppppppppppppppppppppppppppppppppppC CppppppppppppppppppppppppppppppppppppppppppC CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC The C's represent browser chrome, the s's the subscription UI, and the p's represent the feed preview. In this model, the subscription UI would actually be part of the browser chrome, not part of the feed preview at all. The subscription UI would only appear when needed, sort of like how info-bars appear now. Another alternative might be: CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC CssssssssssssssssssssssssssssssssssssssssssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssssssssssssssssssssssssssssssssssssssssssC CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC In this scheme, a privileged page (call it "about:subscribe") would host the subscription controls and would contain another, unprivileged about:feeds page inside an IFRAME. One advantage of this scheme is that it simplifies the "don't call us, we'll call you" approach to inserting the feed preview into the feed preview page.

I vote for the last approach with the "about:subscribe" page and iframe. I don't think we should remove the feature altogether since web developers and users whose feeds are behind firewalls are unable to use Google Reader and the other services we offer by default to test their feeds. I am happy to help with this and participate in https://wiki.mozilla.org/Firefox/Projects/Unbreak_RSS but I don't have a lot of time to devote to it right now, and I see this as fairly impossible for 3.6 anyway (certainly if I'm taking it on...), but it should be possible for 3.7 or later.

No, I don't need 4 reviewers. However, I'm time constrained here and I'll take the first one I can get. This patch is a partial implementation of the "Hollywood Principle" design I proposed in comment #15. * The globally-accessible BrowserFeedWriter constructor is now gone. The FeedWriter class has been stripped down to an empty husk. I will remove it altogether as soon as I can figure how to do so without breaking FeedWriter.js initialization. * The new FeedHandler singleton object watches EndDocumentLoad for pages that can be identified as the "about:feeds" page. For these pages, the feed content is pulled from the cache, converted to JSON format, and then the writeContent() function inside the about:feeds page is called via sandbox. This part of the design is mostly complete, and it is intended to be as secure as possible (and as obviously as possible). * The chrome UI controls at the top of the about:feeds page is now handled by a SubscriptionUI object. Most of this code is unchanged from the old design. In order to complete the refactoring, this code needs to be modified to minimize interaction with the page. Ideally, it will make a small number of unsolicited sandboxed calls into the page with data passed as JSON, similar to how feed preview is now being handled. That's not done yet, and that's the major way that this patch is incomplete. * I'd still like to pull the chrome UI out of the about:feeds page altogether, but that may be a bigger change than I have time for. * There's lots of refactoring snarge where I haven't completely cleaned up from the fairly aggressive refactoring I'm doing. I'm not worried about this stuff right now, my primary concern is whether the general design is going to work out. * This code has been tested, but it has not been *well* tested yet. Due to the fairly drastic refactoring, it will need to be thoroughly re-tested before it's all done. * The old code used nsIScriptableUnescapeHTML.parseFragment() to parse HTML/XHTML content embedded in the source feed. This method isn't (as far as I can tell) available to unprivileged JS. I'm just using "innerHTML" (with an IFRAME hack for HTML-without-the-X). This seems to work just fine for the cursory testing I've been doing, but frankly the solution seems *too* easy. There might be deficiencies that I'm not seeing. * The FeedHandler and FeedConvert classes in FeedWriter.js and the JS code inline in subscribe.xhtml are the primary things of interest in this patch. I'll attach the full FeedWriter.js in a moment, since reading the diff is pretty confusing at this point. * There's probably something I'm forgetting.

FeedWriter.js for <<< "Hollywood" design refactoring (WIP), v1 >>> in its entirety, since the diff is pretty cluttered. Start with the FeedHandler class.

(In reply to comment #17) > * The new FeedHandler singleton object watches EndDocumentLoad for pages that > can be identified as the "about:feeds" page. For these pages, the feed content > is pulled from the cache, converted to JSON format, and then the writeContent() > function inside the about:feeds page is called via sandbox. This part of the > design is mostly complete, and it is intended to be as secure as possible (and > as obviously as possible). [Side question: do safe JS object wrappers not make sandboxes redundant?]

"Hollywood" design refactoring (WIP), v2 Same MO as before, I'll take the first reviewer I can get. This patch extends the previous refactoring by isolating the subscription UI elements from the feed preview for extra security. The new patch implements the nested page design proposed at the end of comment #15. That design looks something like this: CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC CssssssssssssssssssssssssssssssssssssssssssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssppppppppppppppppppppppppppppppppppppppssC CssssssssssssssssssssssssssssssssssssssssssC CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC The C's are browser chrome, the s's represent about:subscribe, a privileged page, and the p's represent the unprivileged feed preview page (about:feeds) which is embedded in about:subscribe using a XUL IFRAME element. The IFRAME specifies 'type="content"' which prevents script running inside the IFRAME widow from accessing the chrome layers above. The about:feeds page maps to "subscribe.xhtml" as before, but the XUL for the subscription UI has been removed. It is only responsible for hosting the feed preview now. The about:subscribe page maps to "subscribe-ui.xul" and contains the XUL for the subscription UI. This file is privileged, which shouldn't be a problem since it is isolated from potentially hostile content inside the about:feeds page. This code is still "work in progress". The refactoring isn't complete. Notably, the SubscriptionUI class in FeedWriter.js still does all of its DOM manipulation using a sandbox, which is kind of pointless since the XUL DOM is now privileged. The sandbox usage is pervasive through that class and adds a lot of complexty, so it would be nice to remove it. As before, there's still a lot of refactoring snarge left over. The code could use use a thorough cleanup pass and such a pass would likely turn up other issues (refactoring opportunities, etc.). It could also benefit from some comments, especially comments calling out the specifics of the security architecture. With the subscription UI XUL running privileged, we need to verify that unsafe user content is not being injected into it. If we're injecting anything at all, it's likely only simple strings like the feed title and feed URL that are being injected. It should be easy to isolate the code that does this and verify that it's handling these strings appropriately. However, I have not attempted to do so yet. One major UI change was necessary with this design. In the old design, the subscription UI and the feed preview are all on one page. If you scroll down in the feed preview page, the subscription UI scrolls off the page. In the new design, the subscription UI is anchored at the top of the window and is always visible. The scrollbar for the feed preview stops abruptly below the subscription UI, which looks pretty weird. It looks even weirder once the feed preview has been scrolled. Then there's the dummy scrollbar hack I used to get the width of the feed preview and the width of the subscription UI to match ("opacity:0" FTW). I think these issues could be fixed with a fairly simple UI redesign. The new scheme doesn't work properly if RSS is viewed in an IFRAME (i.e. the about:subscribe page itself would be in an IFRAME). I think it might be sufficient to only display the feed preview in a case like this, and just add a context menu option to get to the subscription UI. With the about:subscribe page running privileged, then arguably that page shouldn't display in an IFRAME at all. Currently it displays but just doesn't work. I still think its worth thinking about pulling the subscription UI out of the page altogether, and moving it to browser chrome, where it will just pop up when it's needed. I have not fully tested the subscription UI since I've made these changes. In fact I'm not entirely sure how to test it thoroughly since it looks like some of the UI is feed dependent and I haven't found the right feeds to trigger it.

FeedWriter.js for <<< "Hollywood" design refactoring (WIP), v2 >>> in its entirety, since the diff is pretty cluttered. Start with the FeedHandler class.

(In reply to comment #20) > The IFRAME specifies 'type="content"' which prevents script running inside > the IFRAME window from accessing the chrome layers above. That's incorrect. The <browser> in the Firefox chrome specifies type="content" which makes it difficult (even with UXP privileges) for script running in the tab from accessing the chrome layers. However it is not possible for pages loaded into tabs from protecting themselves from their frames in the same way. Obviously the usual XSS protection still applies.

(In reply to comment #22) > (In reply to comment #20) > > The IFRAME specifies 'type="content"' which prevents script running inside > > the IFRAME window from accessing the chrome layers above. > That's incorrect. The <browser> in the Firefox chrome specifies type="content" > which makes it difficult (even with UXP privileges) for script running in the > tab from accessing the chrome layers. However it is not possible for pages > loaded into tabs from protecting themselves from their frames in the same way. > > Obviously the usual XSS protection still applies. I suppose that argues for carrying the design one step further and pulling the subscription UI out of the page altogether. I think that should be relatively easy to do, but I haven't investigated it yet.

bz: Review at your leisure. If you think someone else might be a more suitable reviewer, feel free to delegate. I've done a bunch of code cleanup since v2. In particular, I've removed all use of the sandbox from the SubscriptionUI class. Since the SubscriptionUI XUL elements are in a separate document from the feed preview, the theory is that they can now be safely manipulated without the extra indirection. The current design relies on the subscribe-ui.xul (about:subscribe) page being safely segregated from the feed preview in subscribe.xhtml (about:feeds), which it displays in an IFRAME. However, Neil states in comment #22 that the only real security here is the normal XSS protection. I'm assuming that that's not enough, so I'll take a look at pulling the subscription UI out altogether, and making it a part of the browser chrome (i.e. pop-up info-bar-like). I think that should be pretty easy. I also fixed a couple of bugs, one introduced in the previous refactoring, and one that was pre-existing (albeit pretty minor). The code could benefit from more cleanup, but it's much improved over v2.

What's in this patch: 1. A "don't call us, we'll call you" AKA ("Hollywood") design -- with this design the about:feeds page no longer needs to "call out" to announce its presence, and the old BrowserFeedWriter object is now gone. 2. The XUL UI is now completely segregated from the RSS content -- the XUL UI that used to live in the about:feeds document along with the injected RSS content now lives inside of browser.xul; the RSS content is still displayed in the about:feeds page. The Hollywood design is described in comment #17, and this part of the code hasn't changed substantially since the v1 patch, with one exception: The event handling code which watches for the load event now uses the "DOMContentLoaded" event rather than the poorly documented "EndDocumentLoad" event. (The former also works for documents in frames and iframes, while the latter does not). The big change in this patch is that the XUL UI has been moved from the RSS preview page into the browser chrome. This has a number of implications: 1. The XUL UI does not scroll when the feed document is scrolled -- it always remains at the top of the window. By necessity the XUL UI reduces the height of the feed preview window. This effect will obviously be more pronounced if the overall height of the full window is already very small. 2. Since the XUL UI is part of the browser chrome, it is impractical to display it for feed preview pages in frames or iframes. The solution here is the "ribbon UI", which is only displayed for feed preview pages in frames or iframes. The ribbon UI simply allows the user to display the feed preview as a top level document where the normal XUL UI can be used. (The ribbon UI does not need to be privileged.) 3. The JavaScript code which manages the XUL UI no longer has to treat it like it's hostile content. This code previously used sandboxing every time it manipulated the UI, which meant dozens of places across hundreds of lines of code. This sandboxing is now not necessary and has been removed. This change is the primary advantage of separating the XUL UI from the feed preview page. There are some questions about the patch that need to be answered before proceeding: 1. Does the new design reduce the number of ways that RSS feed preview can be used as an avenue for security exploits? 2. Does the new design make it easier to inspect the code for security exploits? 3. Can we live with the new segregated UI design in light of the issues listed above? Assuming the answer to the questions above is all "yes", what needs to be done to complete the patch? * Some parts of the implementation are still a little rough. The event handling code in FeedHandler.js feels overly complicated, and the way the XUL UI is piggybacked on the notification UI is pretty ugly. I'm sure there are better ways to do both and I could use some expert advice on how to improve them. * A proper UI review is really needed. The old XUL UI layout doesn't feel right with the new segregated UI. For example, the UI could be compressed vertically pretty easily, and since it now steals vertical space from the window rather than the feed preview document, this vertical space is especially valuable. Also the ribbon UI was a quick and dirty proof of concept. It will need UI sign-off and redesign by an actual expert. The "Ribbon UI" The ribbon UI is a minimal HTML UI which is displayed only when a feed preview document is not top-level, i.e. displayed in a frame or iframe. The ribbon displays the feed name and a "Subscribe Now" button. This button simply reloads the feed preview page at the top level so that the regular XUL UI can be displayed. The button functionality is implemented using unprivileged JavaScript capabillities, too wit: window.top.location = location.href The ribbon UI location is fixed at the top of the window (position:fixed), but this could be changed. Notes, in no particular order: * I'm assuming that feed previews will normally be displayed at the top level and previews inside a frame or iframe will be an edge case. We should be able to handle such feed previews in a practical fashion, but we don't need to be particularly elegant. The ribbon UI seems like it meets this requirement, and doesn't require any special JavaScript privileges in the feed preview page. * One advantage of the event-based approach to identifying feed pages used in the FeedHandler object is that it requires no supporting code in browser.js. Maybe this is the wrong way to go, but then there's already too much stuff in browser.js and I'm loathe to add more. * The event handling code seems like it's more complicated than it needs to be. It also attaches to every top-level XUL window which is probably overkill. It adds some overhead every time DOMContentLoaded fires, which may be inconsequential or may not. * The window containing browser.xul seems to generate multiple "unload" events, the first one occurring before the first "load" event. I found that both surprising and inconvenient. (And it might just be a sign that I am doing something wrong.) * I don't know how the new design affects Fennec, SeaMonkey, or Thunderbird, or if it effects them at all. * The XUL UI is actually defined in tabbrowser.xml. It is not defined using XBL although maybe it should be. The XUL UI is done as a template and new instances are created every time a new RSS feed preview page is encountered. This allows the JavaScript code which manages the XUL UI to manage it more or less the same way as it did when the XUL UI lived in the feed preview page. I probably wouldn't have written the code this way if I'd written it from scratch. I'm not sure what the cleanest solution is here, but I know the current one is not it. * I'm not sure what, if anything, the function NSGetModule(cm, file) XPCOMUtils.generateModule([]); at the end of FeedWriter.js does, but I seem to need it. * There is now no FeedWriter class in "FeedWriter.js", and what's left of the old FeedWriter code is now in "subscribe.xhtml". * Note the discussion of HTML parsing at the end of comment #17. * Note that the screenshots in attachment 412329 [details] and attachment 412330 [details] are still accurate even though the underlying implementation has changed quite a lot.

Comment on attachment 422840 [details] [diff] [review] "Hollywood" design refactoring and XUL UI refactoring (WIP), v4 Talked about this a bit on IRC the other day - the review here, and the call about whether or not this is a good approach in the world of COWs is mostly, I think, an mconnor question.

Comment on attachment 422840 [details] [diff] [review] "Hollywood" design refactoring and XUL UI refactoring (WIP), v4 I am no longer actively working on Firefox, please find another reviewer.

(In reply to comment #30) > (From update of attachment 422840 [details] [diff] [review]) > I am no longer actively working on Firefox, please find another reviewer. I don't know who is a suitable alternative to mconnor as a reviewer. Any volunteers, or recommendations from the peanut gallery?

First pass: diff -r 6235ed75968a browser/base/content/tabbrowser.xml --- a/browser/base/content/tabbrowser.xml Thu Jan 21 13:28:40 2010 -0800 +++ b/browser/base/content/tabbrowser.xml Thu Jan 21 14:22:12 2010 -0800 @@ -34,50 +34,57 @@ - Paul O’Shannessy <paul@oshannessy.com> - Rob Arnold <tellrob@gmail.com> - - Alternatively, the contents of this file may be used under the terms of - either the GNU General Public License Version 2 or later (the "GPL"), or - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), - in which case the provisions of the GPL or the LGPL are applicable instead - of those above. If you wish to allow use of your version of this file only - under the terms of either the GPL or the LGPL, and not to allow others to - use your version of this file under the terms of the MPL, indicate your - decision by deleting the provisions above and replace them with the notice - and other provisions required by the GPL or the LGPL. If you do not delete - the provisions above, a recipient may use your version of this file under - the terms of any one of the MPL, the GPL or the LGPL. - - ***** END LICENSE BLOCK ***** --> <!DOCTYPE bindings [ <!ENTITY % tabBrowserDTD SYSTEM "chrome://browser/locale/tabbrowser.dtd" > %tabBrowserDTD; +<!ENTITY % globalDTD + SYSTEM "chrome://global/locale/global.dtd"> +%globalDTD; +<!ENTITY % feedDTD + SYSTEM "chrome://browser/locale/feeds/subscribe.dtd"> +%feedDTD; ]> <bindings id="tabBrowserBindings" xmlns="http://www.mozilla.org/xbl" xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:xbl="http://www.mozilla.org/xbl"> <binding id="tabbrowser"> <resources> <stylesheet src="chrome://browser/content/tabbrowser.css"/> + <stylesheet src="chrome://browser/skin/feeds/subscribe.css"/> </resources> <content> <xul:stringbundle anonid="tbstringbundle" src="chrome://browser/locale/tabbrowser.properties"/> <xul:tabbox anonid="tabbox" flex="1" eventnode="document" xbl:inherits="handleCtrlPageUpDown" onselect="if (!('updateCurrentBrowser' in this.parentNode) || event.target.localName != 'tabpanels') return; this.parentNode.updateCurrentBrowser();"> <xul:hbox class="tab-drop-indicator-bar" collapsed="true" ondragover="this.parentNode.parentNode._onDragOver(event);" ondragleave="this.parentNode.parentNode._onDragLeave(event);" ondrop="this.parentNode.parentNode._onDrop(event);"> <xul:hbox class="tab-drop-indicator" mousethrough="always"/> </xul:hbox> <xul:hbox class="tabbrowser-strip" collapsed="true" tooltip="_child" context="_child" anonid="strip" ondragstart="this.parentNode.parentNode._onDragStart(event);" ondragover="this.parentNode.parentNode._onDragOver(event);" ondrop="this.parentNode.parentNode._onDrop(event);" ondragend="this.parentNode.parentNode._onDragEnd(event);" ondragleave="this.parentNode.parentNode._onDragLeave(event);"> <xul:tooltip onpopupshowing="return this.parentNode.parentNode.parentNode.createTooltip(event);"/> @@ -121,40 +128,82 @@ tbattr="tabbrowser-multiple" oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode; tabbrowser.removeTab(tabbrowser.mContextTab);"/> </xul:menupopup> <xul:tabs class="tabbrowser-tabs" flex="1" anonid="tabcontainer" setfocus="false" onclick="this.parentNode.parentNode.parentNode.onTabClick(event);" ondblclick="this.parentNode.parentNode.parentNode.onTabBarDblClick(event);" onclosetab="var node = this.parentNode; while (node.localName != 'tabbrowser') node = node.parentNode; node.removeCurrentTab();"> <xul:tab selected="true" validate="never" onerror="this.removeAttribute('image');" maxwidth="250" width="0" minwidth="100" flex="100" class="tabbrowser-tab" label="&untitledTab;" crop="end"/> </xul:tabs> </xul:hbox> + + <!-- - - - - - - - - - - - - - - - - - - - - --> + <xul:vbox flex="0" style="display:none"> Why is this (flex=0 style)? You could use hidden. + <xul:hbox id="subscribeBody" flex="0"> ditto. + <xul:vbox id="feedHeaderContainer" flex="1"> + <xul:vbox id="feedHeader" dir="&locale.dir;"> I'm pretty sure you don't need to set the direction, it's inherited from the window. + <xul:vbox id="feedIntroText"> + <xul:description id="feedSubscriptionInfo1" /> + <xul:description id="feedSubscriptionInfo2" /> + </xul:vbox> + Nit: trailing spaces here and in many other places. Please fix. + <xul:hbox> + <xul:vbox id="feedSubscribeLine"> + <xul:vbox> + <xul:hbox align="center"> + <xul:description id="subscribeUsingDescription"/> + <xul:menulist id="handlersMenuList" aria-labelledby="subscribeUsingDescription"> + <xul:menupopup menugenerated="true" id="handlersMenuPopup"> + <xul:menuitem id="liveBookmarksMenuItem" label="&feedLiveBookmarks;" class="menuitem-iconic" image="chrome://browser/skin/page-livemarks.png" selected="true"/> The image should be set on the theme side. + <xul:menuseparator/> + </xul:menupopup> + </xul:menulist> + </xul:hbox> + <xul:hbox> + <xul:checkbox id="alwaysUse" checked="false"/> checked=false is the default state. ... + </xul:vbox> + </xul:vbox> + I'm pretty sure you could simplify the entire hierarchy. diff -r 6235ed75968a browser/components/feeds/content/subscribe.xhtml --- a/browser/components/feeds/content/subscribe.xhtml Thu Jan 21 13:28:40 2010 -0800 +++ b/browser/components/feeds/content/subscribe.xhtml Thu Jan 21 14:22:12 2010 -0800 @@ -7,74 +7,275 @@ %htmlDTD; <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd"> %globalDTD; Could be removed now. + <table id="ribbon" style="display:none;position:fixed;top:0;left:0;width:100%;color:white;background-color:darkblue;height:4em;"> In the theme. + <tr> + <td style="vertical-align:middle;width:0;white-space:nowrap;font-size:160%;"> ditto, and in every other place, if any. + <td style="vertical-align:middle;"> + <xul:button onclick="window.top.location = location.href"> + &feedSubscribeNow; + </xul:button> Why don't you use the attribute? why don't use a html button? + <script type="application/x-javascript"> + <![CDATA[ + + function writeContent(jsonFeed) { + writeFeedContent(jsonFeed); + } + + function writeFeedContent(jsonFeed) { + + var feedWrapper = JSON.parse(jsonFeed); + + if (!feedWrapper) + return; + + // If this is not the top-most window, then show the "ribbon" UI. + if (window != window.top) { To avoid confusion, please change this from top-most to top-most-content. + var entries = feedWrapper.entries; + Prefer let in new code (everywhere). + // Build the actual feed content + Nit: remove the empty line + // If the entry has a title, make it a link + if (entry.title) { + var a = document.createElement("a"); + a.appendChild(document.createTextNode(entry.title)); + + // Entries are not required to have links, so entry.link can be null. + if (entry.link) + a.setAttribute("href", entry.link); + + var title = document.createElement("h3"); + title.appendChild(a); + + if (entry.updated) { + var dateDiv = document.createElement("div"); + dateDiv.className = "lastUpdated"; + dateDiv.textContent = entry.updated; + title.appendChild(dateDiv); + } + + entryContainer.appendChild(title); Please create a separate function for that. + var body = document.createElement("div"); Please find a better name for this variable. + var summary = entry.summary || entry.content; + var docFragment = null; + if (summary) { + const XML_NS = "http://www.w3.org/XML/1998/namespace" Nit: missing semicolon. + // If the entry doesn't have a title, append a # permalink + // See http://scripting.com/rss.xml for an example Nit: missing period. + if (!entry.title && entry.link) { + var a = document.createElement("a"); + a.appendChild(document.createTextNode("#")); + a.setAttribute("href", entry.link); + body.appendChild(document.createTextNode(" ")); + body.appendChild(a); + } Or maybe you should create a generic function for creating links... + + feedContent.appendChild(entryContainer); + feedContent.appendChild(clearDiv); + + } Nit: Remove the empty line (and in everywhere else like this) + } + + /** + * Writes the title image into the preview document if one is present. + * @param container + * The feed container + */ + function setTitleImage(titleImage) { + + // Set up the title image (supplied by the feed) + if (titleImage.url) { + var feedTitleImage = document.getElementById("feedTitleImage"); + feedTitleImage.setAttribute("src", titleImage.url); Use the src property. + } + + var feedTitleLink = document.getElementById("feedTitleLink"); + if (titleImage.title) { + feedTitleLink.setAttribute('title', titleImage.title); + } + if (titleImage.link) { + feedTitleLink.setAttribute("href", titleImage.link); + } + Nit: remove braces around single-line blocks. + + function setTitleText(title, subtitle) { As we move this code, please change the code style for arguments to afoot, now that beng doesn't care. diff -r 6235ed75968a browser/components/feeds/src/FeedWriter.js --- a/browser/components/feeds/src/FeedWriter.js Thu Jan 21 13:28:40 2010 -0800 +++ b/browser/components/feeds/src/FeedWriter.js Thu Jan 21 14:22:12 2010 -0800 if (shouldLog) dump("*** Feeds: " + str + "\n"); + + ;;print("*** Feeds: " + str); hrm? +function FeedHandler() { ... + + function handleLoad(event) { + chromeWin.removeEventListener("load", handleLoad, false); + chromeWin.addEventListener("DOMContentLoaded", handleContentLoaded, false); + chromeWin.addEventListener("unload", handleUnload, false); I prefer an implementation of nsIDOMEventHandler within the object. + var observerService = Cc["@mozilla.org/observer-service;1"] + .getService(Ci.nsIObserverService); + var observer = {observe: function (aWindow, aTopic) { attachTo(aWindow); } }; + observerService.addObserver(observer, "toplevel-window-ready", false); Could be implemented as part of the object as well. + var feedDescription = { + feedTitle: feedTitle, + feedSubtitle: feedSubtitle, + feedType: feedType, + feedURI: feedURI + } Nit: missing semicolon. + // Given a content DOM window, returns the chrome window it's in. + _getChromeWindow: function FH_getChromeWindow(aWindow) { + var chromeWin = aWindow + .QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIWebNavigation) + .QueryInterface(Ci.nsIDocShellTreeItem) + .rootTreeItem + .QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIDOMWindow) + .QueryInterface(Ci.nsIDOMChromeWindow); + return chromeWin; + }, We should move this stills to a resource file at some point… + getStringProperty: function FC_getStringProperty(propertyBag, key, defaultValue) { + if (propertyBag.hasKey(key)) + return propertyBag.getPropertyAsAString(key); + else No else after return + +function SubscriptionUI(chromeWin, feedWindow, feedDescription) { + + this._feedWindow = feedWindow; + this._feedDescription = feedDescription; + this._feedURI = feedDescription.feedURI; + this.feedTitle = feedDescription.feedTitle; + this.feedSubtitle = feedDescription.feedSubtitle; + this.feedType = feedDescription.feedType; + this._chromeDoc = chromeWin.document; + + var self = this; + feedWindow.addEventListener("unload", { handleEvent: function (aEvent) { self.cleanup(aEvent); } }, false); + See my previous comment about event handles. + var tabBrowser = this._chromeDoc.getElementById("content").wrappedJSObject; Er, why do we need to use wrappedJSObject within chrome? + var noteBox = tabBrowser.getNotificationBox(null); + var othersBox = chromeWin.document.getAnonymousElementByAttribute(noteBox, "anonid", "others"); Add a property to notifcationbox if you really need that (in a dependent bug, neil should review). _setCheckboxCheckedState: function FW__setCheckboxCheckedState(aCheckbox, aValue) { - // see checkbox.xml, xbl bindings are not applied within the sandbox! - this._contentSandbox.checkbox = aCheckbox; - var codeStr; var change = (aValue != (aCheckbox.getAttribute('checked') == 'true')); - if (aValue) - codeStr = "checkbox.setAttribute('checked', 'true'); "; - else - codeStr = "checkbox.removeAttribute('checked'); "; + if (aValue) { + aCheckbox.setAttribute('checked', 'true'); + } else { + aCheckbox.removeAttribute('checked'); + } You could simplify this to use the checked property if (change) { - this._contentSandbox.document = this._document; - codeStr += "var event = document.createEvent('Events'); " + - "event.initEvent('CheckboxStateChange', true, true);" + - "checkbox.dispatchEvent(event);" + var event = this._chromeDoc.createEvent('Events'); + event.initEvent('CheckboxStateChange', true, true); + checkbox.dispatchEvent(event); This isn't needed either. _initMenuItemWithFile: function(aMenuItem, aFile) { - this._contentSandbox.menuitem = aMenuItem; - this._contentSandbox.label = this._getFileDisplayName(aFile); - this._contentSandbox.image = this._getFileIconURL(aFile); - var codeStr = "menuitem.setAttribute('label', label); " + - "menuitem.setAttribute('image', image);" - Cu.evalInSandbox(codeStr, this._contentSandbox); + var label = this._getFileDisplayName(aFile); + var image = this._getFileIconURL(aFile); + // Q: Empirically aMenuItem may be null. Why is that? Use js asserts and check! + if (aMenuItem) { + aMenuItem.setAttribute('label', label); + aMenuItem.setAttribute('image', image); Change this to use xbl properties. diff -r 6235ed75968a browser/themes/pinstripe/browser/feeds/subscribe.css --- a/browser/themes/pinstripe/browser/feeds/subscribe.css Thu Jan 21 13:28:40 2010 -0800 +++ b/browser/themes/pinstripe/browser/feeds/subscribe.css Thu Jan 21 14:22:12 2010 -0800 +#subscribeBody { + border: 1px solid THreeDShadow; ThreeDShadow #feedHeader { border: 1px solid ThreeDShadow; - -moz-border-radius: 10px; + -moz-border-radius: 0px 0px 10px 10px; This isn't RTL-friendly, but I'm almost sure this cannot be RTL yet (if so, just add a comment). diff -r 6235ed75968a toolkit/content/widgets/notification.xml --- a/toolkit/content/widgets/notification.xml Thu Jan 21 13:28:40 2010 -0800 +++ b/toolkit/content/widgets/notification.xml Thu Jan 21 14:22:12 2010 -0800 <binding id="notificationbox"> <content> <xul:stack xbl:inherits="hidden=notificationshidden"> <xul:spacer/> <children includes="notification"/> </xul:stack> + <xul:vbox anonid="others"> As I said above, this should be exposed by a property and reviewed by Neil.

(In reply to comment #32) Thanks Mano. I'll try to address these issues today. However, in thinking about things, it occurs to me that there's been no sign-off on the high-level architecture here. This is pretty important because this patch is predicated on the idea that the architecture change really does make RSS preview more secure (or at least easier to verify). Obviously I think it does, but I'm hardly objective. So we need a qualified, disinterested party to comment. It seems like mconnor or gavin would be the best choices here, and mconnor is out of the running and gavin is always busy...

(In reply to comment #32) > > + > + function setTitleText(title, subtitle) { > > As we move this code, please change the code style for arguments to afoot, now > that beng doesn't care. What do you mean by "afoot"?

aFoo

Oh, I thought you've discussed this model with the security peers first (in particular, with dveditz and bz)...

(In reply to comment #32) > First pass: > > diff -r 6235ed75968a browser/base/content/tabbrowser.xml > --- a/browser/base/content/tabbrowser.xml Thu Jan 21 13:28:40 2010 -0800 > +++ b/browser/base/content/tabbrowser.xml Thu Jan 21 14:22:12 2010 -0800 > @@ -34,50 +34,57 @@ > - Paul O’Shannessy <paul@oshannessy.com> > - Rob Arnold <tellrob@gmail.com> > - > - Alternatively, the contents of this file may be used under the terms of > - either the GNU General Public License Version 2 or later (the "GPL"), or > - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), > - in which case the provisions of the GPL or the LGPL are applicable > instead > - of those above. If you wish to allow use of your version of this file > only > - under the terms of either the GPL or the LGPL, and not to allow others to > - use your version of this file under the terms of the MPL, indicate your > - decision by deleting the provisions above and replace them with the > notice > - and other provisions required by the GPL or the LGPL. If you do not > delete > - the provisions above, a recipient may use your version of this file under > - the terms of any one of the MPL, the GPL or the LGPL. > - > - ***** END LICENSE BLOCK ***** --> > > <!DOCTYPE bindings [ > <!ENTITY % tabBrowserDTD SYSTEM "chrome://browser/locale/tabbrowser.dtd" > > %tabBrowserDTD; > +<!ENTITY % globalDTD > + SYSTEM "chrome://global/locale/global.dtd"> > +%globalDTD; > +<!ENTITY % feedDTD > + SYSTEM "chrome://browser/locale/feeds/subscribe.dtd"> > +%feedDTD; > ]> > > <bindings id="tabBrowserBindings" > xmlns="http://www.mozilla.org/xbl" > > xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > xmlns:xbl="http://www.mozilla.org/xbl"> > > <binding id="tabbrowser"> > <resources> > <stylesheet src="chrome://browser/content/tabbrowser.css"/> > + <stylesheet src="chrome://browser/skin/feeds/subscribe.css"/> > </resources> > > <content> > <xul:stringbundle anonid="tbstringbundle" > src="chrome://browser/locale/tabbrowser.properties"/> > <xul:tabbox anonid="tabbox" flex="1" eventnode="document" > xbl:inherits="handleCtrlPageUpDown" > onselect="if (!('updateCurrentBrowser' in this.parentNode) > || event.target.localName != 'tabpanels') return; > this.parentNode.updateCurrentBrowser();"> > <xul:hbox class="tab-drop-indicator-bar" collapsed="true" > ondragover="this.parentNode.parentNode._onDragOver(event);" > > ondragleave="this.parentNode.parentNode._onDragLeave(event);" > ondrop="this.parentNode.parentNode._onDrop(event);"> > <xul:hbox class="tab-drop-indicator" mousethrough="always"/> > </xul:hbox> > <xul:hbox class="tabbrowser-strip" collapsed="true" tooltip="_child" > context="_child" > anonid="strip" > > ondragstart="this.parentNode.parentNode._onDragStart(event);" > ondragover="this.parentNode.parentNode._onDragOver(event);" > ondrop="this.parentNode.parentNode._onDrop(event);" > ondragend="this.parentNode.parentNode._onDragEnd(event);" > > ondragleave="this.parentNode.parentNode._onDragLeave(event);"> > <xul:tooltip onpopupshowing="return > this.parentNode.parentNode.parentNode.createTooltip(event);"/> > @@ -121,40 +128,82 @@ > tbattr="tabbrowser-multiple" > oncommand="var tabbrowser = > this.parentNode.parentNode.parentNode.parentNode; > > tabbrowser.removeTab(tabbrowser.mContextTab);"/> > </xul:menupopup> > > <xul:tabs class="tabbrowser-tabs" flex="1" > anonid="tabcontainer" > setfocus="false" > > onclick="this.parentNode.parentNode.parentNode.onTabClick(event);" > > ondblclick="this.parentNode.parentNode.parentNode.onTabBarDblClick(event);" > onclosetab="var node = this.parentNode; > while (node.localName != 'tabbrowser') > node = node.parentNode; > node.removeCurrentTab();"> > <xul:tab selected="true" validate="never" > onerror="this.removeAttribute('image');" > maxwidth="250" width="0" minwidth="100" flex="100" > class="tabbrowser-tab" label="&untitledTab;" crop="end"/> > </xul:tabs> > </xul:hbox> > + > + <!-- - - - - - - - - - - - - - - - - - - - - --> > + <xul:vbox flex="0" style="display:none"> > > Why is this (flex=0 style)? You could use hidden. Fixed. > + <xul:hbox id="subscribeBody" flex="0"> > > ditto. Fixed. > + <xul:vbox id="feedHeaderContainer" flex="1"> > + <xul:vbox id="feedHeader" dir="&locale.dir;"> > > I'm pretty sure you don't need to set the direction, it's inherited from the > window. Fixed. > + <xul:vbox id="feedIntroText"> > + <xul:description id="feedSubscriptionInfo1" /> > + <xul:description id="feedSubscriptionInfo2" /> > + </xul:vbox> > + > > Nit: trailing spaces here and in many other places. Please fix. Fixed. > + <xul:hbox> > + <xul:vbox id="feedSubscribeLine"> > + <xul:vbox> > + <xul:hbox align="center"> > + <xul:description id="subscribeUsingDescription"/> > + <xul:menulist id="handlersMenuList" > aria-labelledby="subscribeUsingDescription"> > + <xul:menupopup menugenerated="true" > id="handlersMenuPopup"> > + <xul:menuitem id="liveBookmarksMenuItem" > label="&feedLiveBookmarks;" class="menuitem-iconic" > image="chrome://browser/skin/page-livemarks.png" selected="true"/> > > The image should be set on the theme side. I don't think this feature was ever fully themeable. Anyway, let's wait for UI review before addressing this, since I think we'll want to make some adjustments to the UI anyway. > + <xul:menuseparator/> > + </xul:menupopup> > + </xul:menulist> > + </xul:hbox> > + <xul:hbox> > + <xul:checkbox id="alwaysUse" checked="false"/> > > checked=false is the default state. Fixed. > ... > + </xul:vbox> > + </xul:vbox> > + > > I'm pretty sure you could simplify the entire hierarchy. Probably. I was trying to preserve the original XUL as much as possible. Again, I think it's worthwhile to wait for UI review before thinking about this. > > diff -r 6235ed75968a browser/components/feeds/content/subscribe.xhtml > --- a/browser/components/feeds/content/subscribe.xhtml Thu Jan 21 13:28:40 > 2010 -0800 > +++ b/browser/components/feeds/content/subscribe.xhtml Thu Jan 21 14:22:12 > 2010 -0800 > @@ -7,74 +7,275 @@ > %htmlDTD; > <!ENTITY % globalDTD > SYSTEM "chrome://global/locale/global.dtd"> > %globalDTD; > > Could be removed now. Fixed. > + <table id="ribbon" > style="display:none;position:fixed;top:0;left:0;width:100%;color:white;background-color:darkblue;height:4em;"> > > In the theme. See comment above about themeing and UI review. > + <tr> > + <td > style="vertical-align:middle;width:0;white-space:nowrap;font-size:160%;"> > > ditto, and in every other place, if any. See above. > + <td style="vertical-align:middle;"> > + <xul:button onclick="window.top.location = location.href"> > + &feedSubscribeNow; > + </xul:button> > > Why don't you use the attribute? why don't use a html button? Done. Note that the styling on the HTML buttons is slightly different than XUL buttons. This makes the centering feel off. However, until the UI people approve the ribbon design, it's just a proof of concept anyway. > + <script type="application/x-javascript"> > + <![CDATA[ > + > + function writeContent(jsonFeed) { > + writeFeedContent(jsonFeed); > + } > + > + function writeFeedContent(jsonFeed) { > + > + var feedWrapper = JSON.parse(jsonFeed); > + > + if (!feedWrapper) > + return; > + > + // If this is not the top-most window, then show the "ribbon" UI. > + if (window != window.top) { > > To avoid confusion, please change this from top-most to top-most-content. Done. > + var entries = feedWrapper.entries; > + > > Prefer let in new code (everywhere). Done for "subscribe.xhtml". I haven't updated "FeedWriter.js" yet since that file is a mishmash of new and old code, and I don't want to make any more of a hash out of the diff than it already is. > + // Build the actual feed content > + > Nit: remove the empty line Done. > + // If the entry has a title, make it a link > + if (entry.title) { > + var a = document.createElement("a"); > + a.appendChild(document.createTextNode(entry.title)); > + > + // Entries are not required to have links, so entry.link can be > null. > + if (entry.link) > + a.setAttribute("href", entry.link); > + > + var title = document.createElement("h3"); > + title.appendChild(a); > + > + if (entry.updated) { > + var dateDiv = document.createElement("div"); > + dateDiv.className = "lastUpdated"; > + dateDiv.textContent = entry.updated; > + title.appendChild(dateDiv); > + } > + > + entryContainer.appendChild(title); > > Please create a separate function for that. Done. > + var body = document.createElement("div"); > > Please find a better name for this variable. Fixed. > + var summary = entry.summary || entry.content; > + var docFragment = null; > + if (summary) { > + const XML_NS = "http://www.w3.org/XML/1998/namespace" > > Nit: missing semicolon. Fixed. > + // If the entry doesn't have a title, append a # permalink > + // See http://scripting.com/rss.xml for an example > > Nit: missing period. Fixed. > + if (!entry.title && entry.link) { > + var a = document.createElement("a"); > + a.appendChild(document.createTextNode("#")); > + a.setAttribute("href", entry.link); > + body.appendChild(document.createTextNode(" ")); > + body.appendChild(a); > + } > > Or maybe you should create a generic function for creating links... I don't think this would really buy us anything. There are three different places in the code where we're creating anchor elements, and the code is enough different in each place that I don't think there's much common code that can be factored out. > + > + feedContent.appendChild(entryContainer); > + feedContent.appendChild(clearDiv); > + > + } > > Nit: Remove the empty line (and in everywhere else like this) Fixed. > + } > + > + /** > + * Writes the title image into the preview document if one is present. > + * @param container > + * The feed container > + */ > + function setTitleImage(titleImage) { > + > + // Set up the title image (supplied by the feed) > + if (titleImage.url) { > + var feedTitleImage = document.getElementById("feedTitleImage"); > + feedTitleImage.setAttribute("src", titleImage.url); > > Use the src property. Fixed. > + } > + > + var feedTitleLink = document.getElementById("feedTitleLink"); > + if (titleImage.title) { > + feedTitleLink.setAttribute('title', titleImage.title); > + } > + if (titleImage.link) { > + feedTitleLink.setAttribute("href", titleImage.link); > + } > + > > Nit: remove braces around single-line blocks. Fixed. > > + > + function setTitleText(title, subtitle) { > > As we move this code, please change the code style for arguments to afoot, now > that beng doesn't care. Fixed for "subscribe.xhtml", but not yet for "FeedWriter.js". > > diff -r 6235ed75968a browser/components/feeds/src/FeedWriter.js > --- a/browser/components/feeds/src/FeedWriter.js Thu Jan 21 13:28:40 2010 > -0800 > +++ b/browser/components/feeds/src/FeedWriter.js Thu Jan 21 14:22:12 2010 > -0800 > > if (shouldLog) > dump("*** Feeds: " + str + "\n"); > + > + ;;print("*** Feeds: " + str); > > hrm? Left over debug code. Gone. > +function FeedHandler() { > ... > + > + function handleLoad(event) { > + chromeWin.removeEventListener("load", handleLoad, false); > + chromeWin.addEventListener("DOMContentLoaded", handleContentLoaded, > false); > + chromeWin.addEventListener("unload", handleUnload, false); > > I prefer an implementation of nsIDOMEventHandler within the object. Done. > > + var observerService = Cc["@mozilla.org/observer-service;1"] > + .getService(Ci.nsIObserverService); > + var observer = {observe: function (aWindow, aTopic) { attachTo(aWindow); } > }; > + observerService.addObserver(observer, "toplevel-window-ready", false); > > Could be implemented as part of the object as well. Done. > + var feedDescription = { > + feedTitle: feedTitle, > + feedSubtitle: feedSubtitle, > + feedType: feedType, > + feedURI: feedURI > + } > > Nit: missing semicolon. Fixed. > + // Given a content DOM window, returns the chrome window it's in. > + _getChromeWindow: function FH_getChromeWindow(aWindow) { > + var chromeWin = aWindow > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebNavigation) > + .QueryInterface(Ci.nsIDocShellTreeItem) > + .rootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow) > + .QueryInterface(Ci.nsIDOMChromeWindow); > + return chromeWin; > + }, > > We should move this stills to a resource file at some point… Noted. > + getStringProperty: function FC_getStringProperty(propertyBag, key, > defaultValue) { > + if (propertyBag.hasKey(key)) > + return propertyBag.getPropertyAsAString(key); > + else > > No else after return Fixed. > + > +function SubscriptionUI(chromeWin, feedWindow, feedDescription) { > + > + this._feedWindow = feedWindow; > + this._feedDescription = feedDescription; > + this._feedURI = feedDescription.feedURI; > + this.feedTitle = feedDescription.feedTitle; > + this.feedSubtitle = feedDescription.feedSubtitle; > + this.feedType = feedDescription.feedType; > + this._chromeDoc = chromeWin.document; > + > + var self = this; > + feedWindow.addEventListener("unload", { handleEvent: function (aEvent) { > self.cleanup(aEvent); } }, false); > + > > See my previous comment about event handles. Done. > + var tabBrowser = this._chromeDoc.getElementById("content").wrappedJSObject; > > Er, why do we need to use wrappedJSObject within chrome? The problem was really in _getChromeWindow(). Since it was starting with a user content window, a wrapper gets introduced automatically. I moved the ".wrappedJSObject" to _getChromeWindow() where it makes more sense anyway. > + var noteBox = tabBrowser.getNotificationBox(null); > + var othersBox = chromeWin.document.getAnonymousElementByAttribute(noteBox, > "anonid", "others"); > > Add a property to notifcationbox if you really need that (in a dependent bug, > neil should review). I now have a more advanced XBL-based implementation that's a lot cleaner. > _setCheckboxCheckedState: function FW__setCheckboxCheckedState(aCheckbox, > aValue) { > - // see checkbox.xml, xbl bindings are not applied within the sandbox! > - this._contentSandbox.checkbox = aCheckbox; > - var codeStr; > var change = (aValue != (aCheckbox.getAttribute('checked') == 'true')); > - if (aValue) > - codeStr = "checkbox.setAttribute('checked', 'true'); "; > - else > - codeStr = "checkbox.removeAttribute('checked'); "; > + if (aValue) { > + aCheckbox.setAttribute('checked', 'true'); > + } else { > + aCheckbox.removeAttribute('checked'); > + } > > You could simplify this to use the checked property Removed altogether. I'm now just using "checked" directly where this function was being called. > > if (change) { > - this._contentSandbox.document = this._document; > - codeStr += "var event = document.createEvent('Events'); " + > - "event.initEvent('CheckboxStateChange', true, true);" + > - "checkbox.dispatchEvent(event);" > + var event = this._chromeDoc.createEvent('Events'); > + event.initEvent('CheckboxStateChange', true, true); > + checkbox.dispatchEvent(event); > > This isn't needed either. See above. > _initMenuItemWithFile: function(aMenuItem, aFile) { > - this._contentSandbox.menuitem = aMenuItem; > - this._contentSandbox.label = this._getFileDisplayName(aFile); > - this._contentSandbox.image = this._getFileIconURL(aFile); > - var codeStr = "menuitem.setAttribute('label', label); " + > - "menuitem.setAttribute('image', image);" > - Cu.evalInSandbox(codeStr, this._contentSandbox); > + var label = this._getFileDisplayName(aFile); > + var image = this._getFileIconURL(aFile); > + // Q: Empirically aMenuItem may be null. Why is that? > > Use js asserts and check! xxxxxxxxxxxxxxxx > + if (aMenuItem) { > + aMenuItem.setAttribute('label', label); > + aMenuItem.setAttribute('image', image); > > Change this to use xbl properties. "aMenuItem.label" doesn't work, whereas "aMenuItem.setAttribute('label'..." does. Same for "image". It looks like virtually all of our JS code uses setAttrbute for these guys. > diff -r 6235ed75968a browser/themes/pinstripe/browser/feeds/subscribe.css > --- a/browser/themes/pinstripe/browser/feeds/subscribe.css Thu Jan 21 > 13:28:40 2010 -0800 > +++ b/browser/themes/pinstripe/browser/feeds/subscribe.css Thu Jan 21 > 14:22:12 2010 -0800 > +#subscribeBody { > + border: 1px solid THreeDShadow; > > ThreeDShadow Fixed. > #feedHeader { > border: 1px solid ThreeDShadow; > - -moz-border-radius: 10px; > + -moz-border-radius: 0px 0px 10px 10px; > > This isn't RTL-friendly, but I'm almost sure this cannot be RTL yet (if so, > just add a comment). This sets the radius on the lower-left and lower-right corners, which I think is OK regardless of text direction. > diff -r 6235ed75968a toolkit/content/widgets/notification.xml > --- a/toolkit/content/widgets/notification.xml Thu Jan 21 13:28:40 2010 > -0800 > +++ b/toolkit/content/widgets/notification.xml Thu Jan 21 14:22:12 2010 > -0800 > > <binding id="notificationbox"> > <content> > <xul:stack xbl:inherits="hidden=notificationshidden"> > <xul:spacer/> > <children includes="notification"/> > </xul:stack> > + <xul:vbox anonid="others"> > > As I said above, this should be exposed by a property and reviewed by Neil. I've already got a partial implementation which I'll put up for review.

(In reply to comment #36) > Oh, I thought you've discussed this model with the security peers first (in > particular, with dveditz and bz)... My fault. I was happy to get any reviewer, and didn't think so much about what kind of review I needed.

Comment on attachment 428283 [details] [diff] [review] "Hollywood" design refactoring and XUL UI refactoring (WIP), v5 Gavin: As discussed on IRC, this patch really needs an architecture-level review.

* I think your approach is pretty much what i've discussed with mconnor long ago - It should be OK. However, please discuss the design before you code it up next time. * Neither of us (gavin, me) are security experts. Please talk to dveditz.

(In reply to comment #40) > ... However, please discuss the design before you code it > up next time... It's not that I didn't try. Getting any sort of feedback on a 3.7 patch late in the 3.6 development cycle turned out to be rather difficult.

mano: go ahead and review the latest patch. Check-in on this kind of refactoring falls under the "super-review" requirement and we'll fit the security review in at that point

Gavin suggests, via IRC: you should ask mrbkap or bz to comment on the change from exposing the privileged JS object as a global property to the evalInSandbox approach I figured I should summarize this change in a separate comment, so here goes. In comment #15, I proposed changing the way the about:feeds page communicates with privileged code. Here's the relevant section from the comment: The about:feeds page creates a FeedWriter (AKA BrowserFeedWriter) object to communicate with the privileged JavaScript that handles most of the RSS preview and subscription behavior. The FeedWriter can actually be created by any web page, and a page can pass whatever arguments it wants to the various methods exposed by the FeedWriter object. The FeedWriter implementation has been hardened against these sorts of attacks (see bugs 352124, 352124, and 454363). This seems like an excellent opportunity to employ the "Hollywood Principle" (don't call us, we'll call you). This approach was mentioned and dismissed in passing in bug 352124, but it seems to me like it should be easy to do. The major problem seems to be identifying whether a page is really the about:feeds page (the URI is not "about:feeds" but rather the URI of the RSS feed). This seems like it should be a surmountable problem. This needs more investigation, but it seems like this aspect of the design should be pretty easy to fix. This redesign is now implemented in the current patch. This is described in comment #17, to wit: * The globally-accessible BrowserFeedWriter constructor is now gone. The FeedWriter class has been stripped down to an empty husk. I will remove it altogether as soon as I can figure how to do so without breaking FeedWriter.js initialization. * The new FeedHandler singleton object watches EndDocumentLoad for pages that can be identified as the "about:feeds" page. For these pages, the feed content is pulled from the cache, converted to JSON format, and then the writeContent() function inside the about:feeds page is called via sandbox. This part of the design is mostly complete, and it is intended to be as secure as possible (and as obviously as possible). In later patches the FeedWriter class has in fact been removed, and the "DOMContentLoaded" event is now used, rather than "EndDocumentLoad". The basic design hasn't really changed, however.

(In reply to comment #32) > diff -r 6235ed75968a toolkit/content/widgets/notification.xml > --- a/toolkit/content/widgets/notification.xml Thu Jan 21 13:28:40 2010 > -0800 > +++ b/toolkit/content/widgets/notification.xml Thu Jan 21 14:22:12 2010 > -0800 > > <binding id="notificationbox"> > <content> > <xul:stack xbl:inherits="hidden=notificationshidden"> > <xul:spacer/> > <children includes="notification"/> > </xul:stack> > + <xul:vbox anonid="others"> > > As I said above, this should be exposed by a property and reviewed by Neil. Which Neil?

deakin

This patch is a diff against the v4 patch. It shows my first attempt at an XBL implementation of the RSS subscribption UI. It's not 100% complete, and it's got some rough edges, but it already feels like a much cleaner approach than the the crude template approach I used in the v4 patch. I'm putting the patch up as-is for early review. I want to know if this is an appropriate approach for this problem, and if it is, what changes I should make (in the high-level sense) to make an acceptable patch. I'm not yet looking for a low-level line-by-line review on this code. Notes: The XUL for the entire RSS subscription UI is encapsulated in the "RUI" XBL element. However, almost all of the JavaScript which controls the UI is still in FeedWriter.js. (Yes, it needs a better name than "RUI"). The RUI XBL lives in tabbrowser.xml. I'm pretty sure it doesn't belong there, but I'm not sure where to put it. I added two methods to the notificationbox definition in notification.xml: addOtherNotification(elem) removeOtherNotification(elem) These two methods add and remove elements from the "others" vbox added to the notificationbox. This change doesn't show up in the diff since it's also in the v4 diff, so here it is: <binding id="notificationbox"> <content> <xul:stack xbl:inherits="hidden=notificationshidden"> <xul:spacer/> <children includes="notification"/> </xul:stack> <xul:vbox anonid="others"> <-------- container for "other notifications" </xul:vbox> <children/> </content>

Comment on attachment 429218 [details] [diff] [review] XBL implementation of RSS subscription UI (WIP) Hrm, how about having the contents of this binding as part of tabpanel binding (set as hidden by default, thus shouldn't hurt perf much)?

By the way, Neil should review the notification.xml changes. I can review the browser/ changes as much as needed.

Comment on attachment 428283 [details] [diff] [review] "Hollywood" design refactoring and XUL UI refactoring (WIP), v5 Wait, doesn't this patch implement single feeds-UI for *all* tabs?

(In reply to comment #49) > (From update of attachment 428283 [details] [diff] [review]) > Wait, doesn't this patch implement single feeds-UI for *all* tabs? Actually yes it does, and yes, that's wrong. However the patch in attachment 429218 [details] [diff] [review] fixes this so that there will be a separate subscription UI for each tab that's displaying a feed preview page. Only the subscription UI for the currently visible tab (if it's displaying the feed preview page) is shown, so it works like one expects. I should have mentioned that in the attachment comments. Sorry about that.

How so? You're still adding it to tabbrowser and not to tabppanel, as far as I can tell. So, please see comment 47 - I think that's the way to go for now. I would use a dynamic overlay, but that's not possible on anonymous content. Too too bad.

Comment on attachment 429218 [details] [diff] [review] XBL implementation of RSS subscription UI (WIP) About the binding structure: it is still way too complicated. Here are some suggestions plus some other comments: >+ <binding id="RUI"> >+ <content> >+ <xul:vbox flex="1"> You don't need this vbox. >+ <xul:hbox anonid="subscribeBody" class="subscribeBody" flex="0"> You don't need this hbox. Also, what's flex=0? >+ <xul:vbox anonid="feedHeaderContainer" flex="1"> >+ <xul:vbox anonid="feedHeader" class="feedHeader" dir="&locale.dir;"> As I told somewhere else, you don't need to set the direction: It's part of the UI, thus it inherits the window direction. Also, and just FYI, setting the dir attribute to rtl on a vertical XUL element (like vbox) will invert the contents order: bottom to top. text-direction: rtl should be used for setting the horizontal direction. >+ <xul:vbox anonid="feedIntroText" class="feedIntroText"> >+ <xul:description anonid="feedSubscriptionInfo1" /> >+ <xul:description anonid="feedSubscriptionInfo2" /> >+ </xul:vbox> >+ >+ <xul:hbox> >+ <xul:vbox anonid="feedSubscribeLine" class="feedSubscribeLine"> >+ <xul:vbox> >+ <xul:hbox align="center"> >+ <xul:description anonid="subscribeUsingDescription"/> >+ <xul:menulist anonid="handlersMenuList" aria-labelledby="subscribeUsingDescription"> >+ <xul:menupopup menugenerated="true" anonid="handlersMenuPopup"> We don't need to set menugenerated anymore AFAICT. >+ <xul:menuitem anonid="liveBookmarksMenuItem" label="&feedLiveBookmarks;" class="menuitem-iconic" image="chrome://browser/skin/page-livemarks.png" selected="true"/> >+ <xul:menuseparator/> >+ </xul:menupopup> >+ </xul:menulist> >+ </xul:hbox> >+ <xul:hbox> >+ <xul:checkbox anonid="alwaysUse" class="alwaysUse" checked="false"/> checked=false is the default state as noted in the previous review. >+ </xul:hbox> >+ <xul:hbox align="center"> >+ <xul:spacer flex="1"/> >+ <xul:button label="&feedSubscribeNow;" anonid="subscribeButton"/> >+ </xul:hbox> >+ </xul:vbox> >+ </xul:vbox> >+ <xul:spacer flex="1"/> Take this spacer up one level, and you can get rid of the containing hbox. >+ </xul:hbox> >+ </xul:vbox> >+ </xul:vbox> >+ >+ <xul:scrollbar anonid="rude-hack" style="opacity:0" orient="vertical"/> >+ </xul:hbox> >+ </xul:vbox> >+ </content> >+ >+ <implementation> For performance, please initialize the filelds as js fields (in the binding ctor). >diff -u b/toolkit/content/widgets/notification.xml b/toolkit/content/widgets/notification.xml >--- b/toolkit/content/widgets/notification.xml Thu Jan 21 14:22:12 2010 -0800 >+++ b/toolkit/content/widgets/notification.xml Fri Feb 19 17:07:17 2010 -0800 >@@ -60,6 +60,26 @@ > </getter> > </property> > >+<method name="addOtherNotification"> >+ <parameter name="elem"/> >+ <body> >+ <![CDATA[ >+ var others = document.getAnonymousElementByAttribute(this, "anonid", "others"); >+ others.appendChild(elem); >+ ]]> >+ </body> >+</method> >+ >+<method name="removeOtherNotification"> >+ <parameter name="elem"/> >+ <body> >+ <![CDATA[ >+ var others = document.getAnonymousElementByAttribute(this, "anonid", "others"); >+ others.removeChild(elem); >+ ]]> >+ </body> >+</method> Please please take that to a dependent bug.

(In reply to comment #51) > How so? You're still adding it to tabbrowser and not to tabppanel, as far as I > can tell. > > So, please see comment 47 - I think that's the way to go for now. I would use > a dynamic overlay, but that's not possible on anonymous content. Too too bad. (In reply to comment #51) > How so? You're still adding it to tabbrowser and not to tabppanel, as far as I > can tell. > > So, please see comment 47 - I think that's the way to go for now. I would use > a dynamic overlay, but that's not possible on anonymous content. Too too bad. In the case where there are multiple tabs showing feed previews, there are also multiple subscription UI elements. However, only one (at most) is displayed at any one time, and it will only be displayed when its respective tab is visible. I'll take a look at the tabpanel approach...

Comment on attachment 428283 [details] [diff] [review] "Hollywood" design refactoring and XUL UI refactoring (WIP), v5 Just so I don't forget, some general nits: 1) Remove unnecessary empty lines. >+ function writeFeedContent(aJsonFeed) { >+ >+ let feedWrapper = JSON.parse(aJsonFeed); >+ >+ if (!feedWrapper) >+ return; ... >+ for (let i = 0; i < enclosures.length; ++i) { >+ >+ let enc = enclosures[i]; >+ >+ if (!enc.url) >+ continue; ... etc. 2) End comments with period. >+ // Fix the margin on the main title, so that the image doesn't run over >+ // the underline etc. 3) >+ <td align="middle"> >+ <button onclick="window.top.location = location.href"> >+ &feedSubscribeNow; >+ </button> Use label. 4) Whenever possible, use let instead of var in new browser/ code.

I'm holding off the rest of this review for now.

(In reply to comment #51) > How so? You're still adding it to tabbrowser and not to tabppanel, as far as I > can tell. > > So, please see comment 47 - I think that's the way to go for now. I would use > a dynamic overlay, but that's not possible on anonymous content. Too too bad. There's no tabpanel to add it to. It turns out that "notificationbox" elements are added directly to the "tabpanels" collection on the tabbrowser. And in fact notificationbox elements contain the browser element that contains the page content, not just the notifications. Here's the code from tabbrowser.addTab: // Add the Message and the Browser to the box var notificationbox = document.createElementNS( "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "notificationbox"); notificationbox.setAttribute("flex", "1"); notificationbox.appendChild(b); b.setAttribute("flex", "1"); this.mPanelContainer.appendChild(notificationbox); (or see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1321) So it looks like my only recourse here is to add it to the notificationbox element, which, in fact, is what I'm already doing. I guess the question is how it should be added. It looks like I can add the RUI element directly into notificationbox content (i.e. in literal XUL), and dispense with adding it dynamically (and get rid of the whole "otherNotifications" thing). The JS code that drives the subscription UI will need to attach and detach from the RUI element, and it will need to show and hide the RUI element as appropriate. Does that sound like what you're thinking?

Personally, I would prefer moving it out of the notification box. The RUI is not a notification. However, since we the notifcationbox dynamically, you should do the same RUI (Actually, I cannot tell why is it done that way. Although "performance," might be the answer, hidden="false" should just work..)..

(In reply to comment #57) > Personally, I would prefer moving it out of the notification box. The RUI is > not a notification. Ordinarily I'd be inclined to agree with you. However, the notificationbox already contains things that are not notifications. Most significantly, it contains the browser element that displays the *web page content*. Try this change to notification.xml to see this demonstrated visually: <bindings id="notificationBindings" xmlns="http://www.mozilla.org/xbl" xmlns:xbl="http://www.mozilla.org/xbl" xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> <binding id="notificationbox"> <content> <xul:stack xbl:inherits="hidden=notificationshidden"> <xul:spacer/> <children includes="notification"/> </xul:stack> <xul:rss-subscription-ui anonid="rssSubscriptionUI" hidden="true"/> <xul:vbox anonid="others"> </xul:vbox> <xul:vbox style="border: solid red 4px"/> <!-- inserted --> <children/> <!-- must be at top level --> <xul:vbox style="border: solid blue 4px"/> <!-- inserted --> </content> The "notificationbox" may sound like it just contains notifications, but given the way it's actually being used, it should really be called "tabpanel". (There is not, as far as I can tell, an actual "tabpanel", but there is a "tabpanels" -- with an "s" -- element.)

(In reply to comment #58) > (There is not, as far as I can tell, an actual "tabpanel", but there is a > "tabpanels" -- with an "s" -- element.) (That is correct. Any element can be a tab panel.)

(In reply to comment #59) > (In reply to comment #58) > > (There is not, as far as I can tell, an actual "tabpanel", but there is a > > "tabpanels" -- with an "s" -- element.) > (That is correct. Any element can be a tab panel.) My primary point is that the notificationbox is not just for notifications, since it hosts web page content as well. (I'd argue it's misnamed because of this, but that's a minor point.) Is the tabpanels element general-purpose, not just dedicated to document tabs?

(In reply to comment #60) > My primary point is that the notificationbox is not just for notifications, > since it hosts web page content as well. (I'd argue it's misnamed because of > this, but that's a minor point.) notificationbox is a message notification area intended to be a box along the edge of another element that provides messages relevant to that element. The tabbrowser uses a <browser> for that other element, but it could be anything. > Is the tabpanels element general-purpose, not just dedicated to document tabs? tabpanels is used to hold the individual panels of a tab box. tabbrowser happens to put browsers in those panels. Other uses, such as preferences dialogs, put specific preference ui in them.

Er, yes, you're all right. I forgot the way this works. So, sorry for changing my opinion in every other comment, but the more I think about it, the more disadvantages I see in the binding solution. For example, extension won't be able to overlay it. An iframe is probably the way to go here.

(In reply to comment #61) > (In reply to comment #60) > > My primary point is that the notificationbox is not just for notifications, > > since it hosts web page content as well. (I'd argue it's misnamed because of > > this, but that's a minor point.) > > notificationbox is a message notification area intended to be a box along the > edge of another element that provides messages relevant to that element. The > tabbrowser uses a <browser> for that other element, but it could be anything. The tabbrowser puts the browser elements *inside* the notificationbox elements, then puts the notificationbox elements inside the tabpanels element. See comment #58. Because of this, I think the notificationbox has to be where the subscription UI has to go, and it's just a question of how exactly to do this. The patch in attachment 429218 [details] [diff] [review] proposes adding a new slot for UI elements other than notfications, and arbitrary elements can be added and removed from this slot. It then uses this facility to display RUI (rss-subscription-ui) elements when they're needed. I'm not saying this is the best way to do this, but it's the cleanest way I could think of, based on the constraints. (See comment #46)

(In reply to comment #62) > Er, yes, you're all right. I forgot the way this works. > > So, sorry for changing my opinion in every other comment, but the more I think > about it, the more disadvantages I see in the binding solution. For example, > extension won't be able to overlay it. An iframe is probably the way to go > here. Where are you proposing to put the iframe? Also, I'm not sure there's any requirement that this UI be overlayable, since it's the existing UI (XUL inside an XHTML webpage) isn't overlayable either.

What's wrong with: <notificationbox> <otherstuff>...</otherstuff> <browser/> </notificationbox>

(In reply to comment #64) > (In reply to comment #62) > Where are you proposing to put the iframe? Within the notification box. > Also, I'm not sure there's any requirement that this UI be overlayable, since it's the existing UI (XUL inside > an XHTML webpage) isn't overlayable either. Most of our UI is overlayable, and we shouldn't make it harder for extensions to extend our ui if we don't have to do so. In general, we shouldn't use xbl for anything but widgets. See the tabs case: the DOM structure of each tab is hidden but the tabs themselves are exposed.

(In reply to comment #65) > What's wrong with: > > <notificationbox> > <otherstuff>...</otherstuff> > <browser/> > </notificationbox> It cannot have ids, unless you mean "iframe" by otherstuff, in which case the ids will be given within the sub document.

(In reply to comment #67) > (In reply to comment #65) > > What's wrong with: > > > > <notificationbox> > > <otherstuff>...</otherstuff> > > <browser/> > > </notificationbox> > > It cannot have ids, unless you mean "iframe" by otherstuff, in which case the > ids will be given within the sub document. If we have an rss-subscription-ui XBL element, the ids are not a problem because the structure will all be in the anonymous nodes, identified by "anonid". It's true that the rss-subscription-ui element would be an unusual "widget", but as long as we don't pollute the real widgets, I'm not sure that's a big problem. (There is still the question of which XBL .xml file the definition would live in.) There are a couple of things I don't like about an iframe solution. One is that the iframe won't auto-size to the content; another is that I end up having to deal with two different load events in JavaScript -- the load event for the RSS preview page and the load event for the subscription UI that will be displayed in the iframe. Neither of these problems are insurmountable, of course.

(In reply to comment #65) > What's wrong with: > > <notificationbox> > <otherstuff>...</otherstuff> > <browser/> > </notificationbox> As it stands now, the browser element has to be the first element, I get an XBL binding error if it's not. I suspect there's JavaScript somewhere that just grabs the first child element and expects it to be the browser. It looks like you can put another element in as the second element (I've just tested with an empty vbox so far). Also note that the contents of the first tab are specified in XUL but subsequent tabs are built up programmatically. Probably not a big deal, but there are two places that would need to be modified.

Do you get an xbl error or a javascript error? You cannot get xbl error due to javascript code... > If we have an rss-subscription-ui XBL element, It's, by no means, "an element", as I tried to explain.

(In reply to comment #70) > Do you get an xbl error or a javascript error? You cannot get xbl error due to > javascript code... > > > If we have an rss-subscription-ui XBL element, > > It's, by no means, "an element", as I tried to explain. I understand your position here, but I respectfully disagree. We can make it an element if we want to. If this makes the overall effort simpler and cleaner, we ought to at least be willing to consider the idea. Nevertheless, I'll put together an iframe-based solution and see how that works out.

Changes in this patch: * The RSS UI now lives in "subscribe.xul", and it's displayed in an iframe inside the notificationbox. * Now handles simultaneous RSS preview in multiple tabs. * Cosmetic changes including whitespace, replacing var with let, etc. * Spent a lot of time tracking down related bugs (see below). Note that ddahl is working on another feature that displays UI inside the notificationbox element. ----- related bugs ----- --- ###!!! ASSERTION: This is unsafe!: 'Error', file /Users/curtisb/dev/firefox-a/src/content/events/src/nsEventDispatcher.cpp, line 486 This bug happens whenever the initially hidden iframe in the notificatonbox is first unhidden. It's somehow related to the menulist, since changing the menulist to a menu makes the assertion go away. The only reliable work-around I've found is to build the menulist programatically and insert it after the iframe is revealed. I have not been able to build a reduced test case, so I have not yet opened a new bug for it. It might be related to bug 538815 or bug 535887. --- ###!!! ASSERTION: Invalid computed width: 'aComputedWidth >= 0', file /Users/curtisb/dev/firefox-a/src/layout/generic/nsHTMLReflowState.cpp, line 229 Bug 551794 - ###!!! ASSERTION: Invalid computed width: 'aComputedWidth >= 0', file /Users/curtisb/dev/firefox-b/src/layout/generic/nsHTMLReflowState.cpp, line 229 This is a regression and I have a regression range in the bug. The bug is present in the existing RSS preview feature. --- ###!!! ABORT: Shouldn't already be blocking: '!mBlockingOnload', file /Users/curtisb/dev/firefox-a/src/content/base/src/nsImageLoadingContent.cpp, line 186 Bug 533454 - ABORT: Shouldn't already be blocking: '!mBlockingOnload' This is an existing bug. I added a simplified test case.

Interdiff showing just the changes from v5 to v6.

ddahl has been exploring a similar notficationbox-based UI in bug 529086 (Create an Interactive Console for debugging web pages).

It looks like the content-document-global-created event implemented in bug 549539 might be a superior alternative to hooking on DOMContentLoaded.

Before I put any more effort into this bug, I need an unequivocal "Yes, we want this" in a comment on the bug, and I need this to come from someone who has (a) actually read all the bug comments and (b) has the standing/authority/credentials to ultimately approve the patch for landing. Notes: By "Yes, we want this", I mean the specific patch I've been working on, not just some hypothetical fix for the bug. I am asking for a *high-level* is-this-the-right-approach review, not a low-level line-by-line code review. I believe there's enough information just in my bug comments to decide whether the *approach* is right. I think it will take about 45 minutes to read all the bug comments, assuming one just skims the line-by-line code review comments. I'm happy to answer questions and offer any clarifications that might be needed. I don't know who the right person to do this high-level review is. It may have been mconnor at one time, but he's no longer actively working on Firefox (see comment #30). The one thing I do know is that whoever this person is, they are very, very busy.

Curtis, I'm really sorry for not being clear about this and for not being very communicate in general. The "high-level" review which you're looking for is a security review (content-chrome interaction). Thus, it should be done by JS peers like jst, mrbkap and bz. Mconnor, gavin, neil, me and any other Fx and Toolkit peer are not the right people to make the decision here. Also, for what's it worth, I completely disagree with comment 42. Indeed it's the kind of patch that should get a super/high-level/security-review before it gets the general line-by-line review.

communicative*

(In reply to comment #77) > Curtis, I'm really sorry for not being clear about this and for not being very > communicate in general. > > The "high-level" review which you're looking for is a security review > (content-chrome interaction). Thus, it should be done by JS peers like jst, > mrbkap and bz. Mconnor, gavin, neil, me and any other Fx and Toolkit peer are > not the right people to make the decision here. Also, for what's it worth, I > completely disagree with comment 42. Indeed it's the kind of patch that should > get a super/high-level/security-review before it gets the general line-by-line > review. I think you and I are largely in agreement here. Also, I should apologize for letting you tear into the low-level review before that made sense. I was just happy to get any sort of review at that point. (I've been requesting review every time I uploaded a work-in-progress patch, but without any luck.) Note that there is also a need to do some sort of UI review here, since the architectural changes require some changes in how the UI works (the "ribbon UI", for instance). I think Firefox and Toolkit peers might have some role to play there.

Just my 2 cents... Getting design review by asking for code review is not a great way to get design review, especially if that's not made clear to reviewers. I know that in my case new patches got uploaded faster than I got a chance to look at old ones, and I assumed the code churn meant that the patches were not in fact ready for review yet.... If there's a design review needed here, the best way to go, imo, is to send mail to whoever you want to do the design review with a pointer to the design notes. Or request review on an attachment that contains the design notes. Or something like that.

(In reply to comment #80) > Just my 2 cents... Getting design review by asking for code review is not a > great way to get design review, especially if that's not made clear to > reviewers. I know that in my case new patches got uploaded faster than I got a > chance to look at old ones, and I assumed the code churn meant that the patches > were not in fact ready for review yet.... Well, yeah, obviously, that didn't work. Anyway, I'm not blaming anyone here. I understand just how busy everyone was trying to ship 3.6. I just wanted to make it clear that I did try to get feedback, and I just failed at it. Ultimately I tried a different tack, and that's how I turned up Mano, before realizing he wasn't the right guy, or at least wasn't the right guy just yet. Now I'm on my third try. > If there's a design review needed here, the best way to go, imo, is to send > mail to whoever you want to do the design review with a pointer to the design > notes. Or request review on an attachment that contains the design notes. Or > something like that. Problem #1 is that I still don't know who that person should be. Mano is suggesting you, mrbkap, or jst. Do I just pick one person, or ask for a volunteer?

There's a lot of people copied on this bug, but the only person who is responsible for it right now is me. This is a problem because I am no longer a Mozilla employee, I'm now just another volunteer, and I have very little standing in the Mozilla development community, and not a whole lot of experience. I can continue to work on the code, but I cannot drive the process through to completion by myself. I need help. I could elaborate on what I mean by "help", but for now let me just direct everybody's attention back to comment #76.

> Problem #1 is that I still don't know who that person should be. I think starting with mrbkap is best, since he has the best feel for our JS wrappers' security features at this point. If he feels that someone else would be better, he will likely tell you. That's assuming that the key thing to review is the pure-JS communication between the chrome and the page. If there's something else going on, I'm quite happy to look at things, if only to be able to better answer your "who is the right reviewer?" question. Regarding comment 76, I can try to dig up the 45 minutes you estimate reading the whole bug requires if needed, but if there's a smaller subset that you feel is good enough to read to get the picture about what your patch is doing, please tell me?

(In reply to comment #83) > > Regarding comment 76, I can try to dig up the 45 minutes you estimate reading > the whole bug requires if needed, but if there's a smaller subset that you feel > is good enough to read to get the picture about what your patch is doing, > please tell me? Sure. First, a quick overview: To the best of our knowledge, all of the security issues in RSS Feed Preview have been fixed. However, due to the number of issues that were encountered, there is a fear that there are other lurking security issues in this code that haven't been identified yet. The goal was to re-design the code to drastically reduce the chance that there were unidentified security issues. My basic principle with the redesign was to drastically reduce the exposure surface, which seems unnecessarily broad the way this feature is currently implemented. I used the following techniques to do this: 1. Eliminated the BrowserFeedWriter object, and changed the design to call *into* the feed preview page (the "Hollywood" design), rather than letting the feed preview page call out. All of the data is passed in as a single JSON string eliminating the possibility of privileged objects leaking into the unprivileged page where they might be vulnerable to an XSS attack. HTML/XHTML embedded in the feed is passed in as raw text; parsing is actually handled inside the unprivileged feed preview page. 2. Moved the XUL for the subscription UI out of the feed preview page (where it's mixed with possibly hostile content from the feed) into the browser chrome, where it's just regular privileged UI. This eliminates the need to sandbox access to the UI elements or rely on wrappers to provide security. This required a minor change to how the subscription UI behaves (it doesn't scroll with page content anymore since it's no longer part of the page). This also required the introduction of the simplified "ribbon UI" for feed previews displayed in frames. All of this is described in more detail in the following comments. There was some evolution of the design in each new attachment, but I think the attachment comments aren't that redundant so it's useful to read them in order. It might be worthwhile to read comment #43 first, however. I skipped the v5 patch since that's just me responding to Mano's first review comments, and there was no significant design change on that attachment. Comment #15: Summary of Possible Approaches Comment #17: "Hollywood" design refactoring (WIP), v1 Comment #20: "Hollywood" design refactoring (WIP), v2 Comment #26: "Hollywood" design refactoring (WIP), v3 Comment #28: "Hollywood" design refactoring and XUL UI refactoring (WIP), v4 Comment #43: Summary of the "Hollywood" design as ultimately implemented Comment #46: XBL implementation of RSS subscription UI (WIP) Comment #72: "Hollywood" design refactoring and XUL UI refactoring (WIP), v6 (If this design sounds like overkill, my response would be "Yes".)

(In reply to comment #83) > > Problem #1 is that I still don't know who that person should be. > > I think starting with mrbkap is best, since he has the best feel for our JS > wrappers' security features at this point. If he feels that someone else would > be better, he will likely tell you. > > That's assuming that the key thing to review is the pure-JS communication > between the chrome and the page. If there's something else going on, I'm quite > happy to look at things, if only to be able to better answer your "who is the > right reviewer?" question. Since the redesign drastically reduces the amount of chrome/page communication, I think evaluating the change from a wrapper perspective is really a small part of the review.

(In reply to comment #85) > > Since the redesign drastically reduces the amount of chrome/page communication, > I think evaluating the change from a wrapper perspective is really a small part > of the review. ... So I think mrbkap might not be the right person for the review. (Sorry, kind of jumped the gun on that last comment.)

FYI, I'm going to be out of town for the next week (March 24-31). I'll still be responding to email/bugmail, but with some latency. I probably won't be on IRC, however.

Would this be benefited or sped by breaking this review into smaller pieces?

(In reply to comment #88) > Would this be benefited or sped by breaking this review into smaller pieces? Right now the sticking point is whether the solution I have proposed and (mostly) implemented is even desirable. I think it will greatly reduce the security risks posed by the current code, but it may also be complete overkill. It may be that the work on security wrappers makes the old scary code not particularly scary any more. I'm not really a security expert and I also don't know much about the wrappers, so I can't say. My goal was to absolutely minimize the reliance on wrappers so that it would be much easier to verify that the code was secure by inspection. But as I say, this may be overkill. Anyway, let me direct everyone's attention back to comment #76 (and maybe comment #84 as well). As I explain there, the next thing that's needed is a high-level review which I think can be done without actually looking at any of my patches, but rather just my bug comments where I explain what I'm trying to do.

As far as I can tell, the only thing blocking this from moving ahead is a lack of a feedback+ on the approach from the right person. Damon, can you find an owner for this review? Some comments say mrbkap.

I poked Blake about this just now, and he says he's not the right person because he's not really a XUL/XBL/JS end user-type hacker, which he thinks is something the reviewer probably needs (in addition to security chops). So who is? Beats me.

The patch needs two things: a) an evaluation as to whether the general approach makes things safer in general b) a review of the functional changes, and the approach's implementation a) is what we're looking to get from mrbkap (or anyone else willing to comment). My general impression is that our wrapper situation is better now than it was when this was filed, and "security whack-a-mole" isn't really an accurate description of our current state - we've not had to make any security-related fixes to this code in a while, as far as I know. I don't really want to suggest throwing away the work Curtis has done based on my general impression, though, so additional feedback here would be valuable. b) can be handled by any number of people once a) has been settled, so it shouldn't scare Blake away!

(In reply to comment #92) > My general impression is that our wrapper situation is better now > than it was when this was filed, and "security whack-a-mole" isn't really an > accurate description of our current state My general impression is that our current wrapper situation makes a lot of the existing whack-a-mole unnecessary and it could just be ripped out. But I failed to get mrbkap to confirm or deny that on IRC.

Given that there may not be a security bug in this code (especially since the work on COWs), does it make sense to go ahead and unhide this bug? Johnath mentions that possibility way back in comment #14. It seems like exposing the bug to a wider audience will either give us more confidence that things are OK as they stand *or* will point out some existing flaw that we really should fix.

Yeah, given that we have no open security bugs against this, it seems like having more eyes on it would be better.

Given that we're no longer sure there's a security bug here anymore, I'm not going to block the release on this. Re-request if this bug's status changes.

Fixed by removing preview code.