Last Comment Bug 846763 - Browser Feed Preview is broken due to xbl_scopes.
: Browser Feed Preview is broken due to xbl_scopes.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.19
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
: 847842 (view as bug list)
Depends on:
Blocks: 834697
  Show dependency treegraph
 
Reported: 2013-03-01 07:23 PST by Philip Chee
Modified: 2013-05-30 13:44 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed


Attachments
Proposed patch (11.54 KB, patch)
2013-03-01 16:49 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
philip.chee: feedback+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2013-03-01 07:23:04 PST
STR:
1. Set dom.xbl_scopes to true
2. Visit http://www.groklaw.net/backend/GrokLaw.rdf
3. Feed preview UI is messed up.

Error Console messages:

Fri Mar 01 2013 22:53:00
Error: ReferenceError: SubscribeHandler is not defined
Source file: chrome://communicator/content/feeds/subscribe.xml
Line: 35

Fri Mar 01 2013 22:53:00
Error: TypeError: this._feedWriter is null
Source file: chrome://communicator/content/feeds/subscribe.js
Line: 18

Flipping dom.xbl_scopes to false and all is good again.

Changing this line:
http://hg.mozilla.org/comm-central/annotate/bead6b9aeb9f/suite/common/feeds/subscribe.xml#l35

From:
SubscribeHandler.init();
to:
window.wrappedJSObject.SubscribeHandler.init();
Allows this to work with dom.xbl_scopes enabled.

Neil says to CC bholley
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2013-03-01 08:27:40 PST
Does SubscribeHandler live in untrusted content?
Comment 2 neil@parkwaycc.co.uk 2013-03-01 11:26:11 PST
OK, so this is how things used to work:

When we sniff a feed, we replace the document with about:feeds which is a "safe" about: page i.e. it runs as content. This page then used to create a BrowserFeedWriter object which is an XPConnect global constructor and init that.

Unfortunately the subscription page includes XBL and so we needed to ensure that the BrowserFeedWriter object wasn't initialised until after the XBL was constructed. We therefore called an init method from the XBL constructor.

It appears that our XBL now runs as chrome, is that correct? In which case, could we not rip out all of the BrowserFeedWriter goop and talk directly to the nsIFeedWriter service? In particular, would the nsIFeedWriter JS component be able to safely invoke our XBL methods?
Comment 3 neil@parkwaycc.co.uk 2013-03-01 11:30:57 PST
(In reply to comment #2)
> Unfortunately the subscription page includes XBL and so we needed to ensure
> that the BrowserFeedWriter object wasn't initialised until after the XBL was
> constructed. We therefore called an init method from the XBL constructor.

Note that Firefox doesn't bother checking that the XBL has been constructed, so it's possible that it will intermittently fail.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-03-01 11:40:16 PST
(In reply to neil@parkwaycc.co.uk from comment #2)
> It appears that our XBL now runs as chrome, is that correct?

It does not. It runs with nsExpandedPrincipal(contentPrincipal), which means it has slightly elevated privileges with respect to the content (including Xray vision), but can't access data from other origins. Additionally, it can't do privileged stuff like talking to Components.
Comment 5 neil@parkwaycc.co.uk 2013-03-01 11:48:26 PST
(In reply to Bobby Holley from comment #4)
> It runs with nsExpandedPrincipal(contentPrincipal), which means
> it has slightly elevated privileges with respect to the content (including
> Xray vision), but can't access data from other origins. Additionally, it
> can't do privileged stuff like talking to Components.

In that case I think I should merge SubscribeHandler into subscribe.xml and make it add the load and unload event handlers.
Comment 6 neil@parkwaycc.co.uk 2013-03-01 16:49:50 PST
Created attachment 720189 [details] [diff] [review]
Proposed patch

* Supported nsIDOMGlobalObjectConstructor so that I don't have to explicitly init() the BrowserFeedWriter object.
* Moved the event handlers from the <body> to the BrowserFeedWriter object.
* Removed the SubscribeHandler object
* Removed the nsIFeedWriter interface

So all the XBL constructor has to do is to create a new BrowserFeedWriter.

(Firefox doesn't actually create the BrowserFeedWriter from its XBL but I thought Gavin might be interested in the simplification anyway.)
Comment 7 Phoenix 2013-03-05 08:44:43 PST
*** Bug 847842 has been marked as a duplicate of this bug. ***
Comment 8 Philip Chee 2013-03-07 09:09:04 PST
Comment on attachment 720189 [details] [diff] [review]
Proposed patch

looks good f=me

> +  // nsIDOMGlobalObjectConstructor
> +  constructor: function constructor(aWindow) {
I've been meaning to ask what passes a window to the constructor?
Comment 9 Philip Chee 2013-03-07 09:58:49 PST
*** Bug 693034 has been marked as a duplicate of this bug. ***
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-13 17:03:45 PDT
(In reply to Philip Chee from comment #8)
> > +  constructor: function constructor(aWindow) {
> I've been meaning to ask what passes a window to the constructor?

The code at http://hg.mozilla.org/mozilla-central/annotate/8e68f4d73ec4/dom/base/nsDOMClassInfo.cpp#l4029.
Comment 11 Ian Neal 2013-03-17 08:46:01 PDT
Comment on attachment 720189 [details] [diff] [review]
Proposed patch

r=me with the relevant changes made to suite/feeds/public/moz.build instead of Makefile.in
Comment 12 neil@parkwaycc.co.uk 2013-03-17 14:40:38 PDT
Pushed comm-central changeset 5d386c556c78.
Comment 13 neil@parkwaycc.co.uk 2013-03-28 17:23:29 PDT
For some reason the file removals got lost when I merged the patch to tip.

Pushed comm-central changeset adf2b7942dfd to address that.
Comment 14 neil@parkwaycc.co.uk 2013-04-02 11:47:29 PDT
Comment on attachment 720189 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): 834697
User impact if declined: Feed preview does not work
Testing completed (on m-c, etc.): Merged to aurora
Risk to taking this patch (and alternatives if risky): Risky alternative is to back out bug 590725 and port Firefox's changes from a bug I can't find instead.
String changes made by this patch: None
Comment 15 neil@parkwaycc.co.uk 2013-04-05 16:11:48 PDT
http://hg.mozilla.org/releases/comm-beta/rev/997fbd08ee62
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-30 13:44:39 PDT
Comment on attachment 720189 [details] [diff] [review]
Proposed patch

(In reply to neil@parkwaycc.co.uk from comment #6)
> (Firefox doesn't actually create the BrowserFeedWriter from its XBL but I
> thought Gavin might be interested in the simplification anyway.)

I filed bug 877834.

Note You need to log in before you can comment on or make changes to this bug.