Closed Bug 846763 Opened 11 years ago Closed 11 years ago

Browser Feed Preview is broken due to xbl_scopes.

Categories

(SeaMonkey :: Feed Discovery and Preview, defect)

Product:
SeaMonkey
Component:
Feed Discovery and Preview
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.17 unaffected, seamonkey2.18 fixed, seamonkey2.19 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.19
Tracking Flags:
Tracking Status
seamonkey2.17 --- unaffected
seamonkey2.18 --- fixed
seamonkey2.19 --- fixed

People

(Reporter: philip.chee, Assigned: neil)

References

Details

Attachments

(1 file)

Proposed patch
11.54 KB, patch
iannbugzilla
: review+
philip.chee
: feedback+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review 
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
Does SubscribeHandler live in untrusted content?
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?
(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.
(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.
(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.
Attached patch Proposed patch — — Splinter Review 
* 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.)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #720189 - Flags: review?(iann_bugzilla)
Attachment #720189 - Flags: feedback?(philip.chee)
Attachment #720189 - Flags: feedback?(gavin.sharp)
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?
Attachment #720189 - Flags: feedback?(philip.chee) → feedback+
(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 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
Attachment #720189 - Flags: review?(iann_bugzilla) → review+
Pushed comm-central changeset 5d386c556c78.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.19
For some reason the file removals got lost when I merged the patch to tip.

Pushed comm-central changeset adf2b7942dfd to address that.
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
Attachment #720189 - Flags: approval-comm-beta?
Attachment #720189 - Flags: approval-comm-beta? → approval-comm-beta+
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.
Attachment #720189 - Flags: feedback?(gavin.sharp)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: