Browser Feed Preview is broken due to xbl_scopes.

RESOLVED FIXED in seamonkey2.19

Status

SeaMonkey
Feed Discovery and Preview
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.19

SeaMonkey Tracking Flags

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

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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?
(Assignee)

Comment 2

4 years ago
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?
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
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.)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #720189 - Flags: review?(iann_bugzilla)
Attachment #720189 - Flags: feedback?(philip.chee)
Attachment #720189 - Flags: feedback?(gavin.sharp)

Updated

4 years ago
Duplicate of this bug: 847842
(Reporter)

Comment 8

4 years ago
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+
(Reporter)

Updated

4 years ago
Duplicate of this bug: 693034
(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

4 years ago
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+
(Assignee)

Comment 12

4 years ago
Pushed comm-central changeset 5d386c556c78.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → seamonkey2.19
(Assignee)

Comment 13

4 years ago
For some reason the file removals got lost when I merged the patch to tip.

Pushed comm-central changeset adf2b7942dfd to address that.
(Assignee)

Comment 14

4 years ago
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?
(Assignee)

Updated

4 years ago
status-seamonkey2.17: --- → unaffected
status-seamonkey2.18: --- → affected
status-seamonkey2.19: --- → fixed

Updated

4 years ago
Attachment #720189 - Flags: approval-comm-beta? → approval-comm-beta+
(Assignee)

Comment 15

4 years ago
http://hg.mozilla.org/releases/comm-beta/rev/997fbd08ee62
status-seamonkey2.18: affected → fixed
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.