Closed
Bug 983845
Opened 11 years ago
Closed 11 years ago
Stop exposing BrowserFeedWriter to the Web
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
10.30 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8391545 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
OK, so the NS_SecurityCompareURIs change breaks all of the things, as can be seen on try. Boris, what's the right way to do this? Note that we need to do something which is compatible with the magic here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/FeedConverter.js#248>
Comment 4•11 years ago
|
||
Your proximate problem is that CheckSameOrigin() throws an exception on the JSContext if the check fails.
But even if we addressed that, I don't think we want to actually change SecurityComparURIs here. Not without a lot of thinking about the attack avenues it opens up.
Instead, I'd probably go with checking that xpc::WindowGlobalOrNull(aGlobal) returns non-null, and then that the resulting principal has a URI which has SchemeIs("about") (to avoid getting long specs in common cases), and spec is "about:feeds".
Assignee | ||
Updated•11 years ago
|
Attachment #8391545 -
Attachment is obsolete: true
Attachment #8391545 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky
https://tbpl.mozilla.org/?tree=Try&rev=7442d8aadeaa
Comment 7•11 years ago
|
||
Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky
>+ void subscribe();
Nobody actually calls subscribe(). Although subscribe.js references it, nobody calls that anyway.
It's actually possible to refactor the code so that you don't actually have to call anything from content at all; see bug 846763. (Unfortunately because nsIXPCScriptable is itself not actually scriptable you can't hide the nsIDOMGlobalObjectConstructor interface from explicit discovery via QueryInterface, and SeaMonkey doesn't hide the other three interfaces it uses, although that's achievable through use of a wrapper object. Also webidl doesn't support nsIDOMGlobalObjectConstructor but if you're able to limit the use of BrowserFeedWriter to about:feeds itself then you could probably switch to nsIDOMGlobalPropertyInitializer).
Of course, there may be a better way for about:feeds to notify chrome; I'm open to suggestions.
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #7)
> Comment on attachment 8391759 [details] [diff] [review]
> Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web;
> r=bzbarsky
>
> >+ void subscribe();
> Nobody actually calls subscribe(). Although subscribe.js references it,
> nobody calls that anyway.
>
> It's actually possible to refactor the code so that you don't actually have
> to call anything from content at all; see bug 846763. (Unfortunately because
> nsIXPCScriptable is itself not actually scriptable you can't hide the
> nsIDOMGlobalObjectConstructor interface from explicit discovery via
> QueryInterface, and SeaMonkey doesn't hide the other three interfaces it
> uses, although that's achievable through use of a wrapper object. Also
> webidl doesn't support nsIDOMGlobalObjectConstructor but if you're able to
> limit the use of BrowserFeedWriter to about:feeds itself then you could
> probably switch to nsIDOMGlobalPropertyInitializer).
Please file a follow-up, but I won't probably take that. My only interest here is to stop expose BrowserFeedWriter to the Web, which this patch accomplishes.
Assignee | ||
Comment 9•11 years ago
|
||
Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky
Seems like I forgot to ask for review here or something!
Attachment #8391759 -
Flags: review?(bzbarsky)
Comment 10•11 years ago
|
||
Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky
>+++ b/browser/components/feeds/src/FeedWriterEnabled.h
>+ NS_ENSURE_TRUE(uri, false);
Please don't warn if !uri. That just means system principal, which is a totally normal thing to have around. Just return false.
>+ bool isAbout = false;
>+ uri->SchemeIs("about", &isAbout);
>+ if (!isAbout) {
Document that this is an optimization to avoid getting long specs?
>+++ b/dom/bindings/Bindings.conf
>+'BrowserFeedWriter': {
>+ 'headerFile': 'mozilla/FeedWriterEnabled.h',
This seems fishy to me. In particular, that's not where BrowserFeedWriter is defined...
It may be simpler to do Func="mozilla::FeedWriterEnabled::IsEnabled", which will automatically make us include "mozilla/FeedWriterEnabled.h" for the function. And then you can just make FeedWriterEnabled a namespace, or a class with a static method.
>+++ b/dom/webidl/BrowserFeedWriter.webidl
>+ void init(WindowProxy aWindow);
We could drop this by implementing QI to nsIDOMGlobalPropertyInitializer on the component (in which case the WebIDL machinery will automatically call init() with a window and we could remove the explicit call). Either way.
>+ void subscribe();
This is unused, right? Why add it?
>+++ b/dom/webidl/moz.build
>+if CONFIG['MOZ_BUILD_APP'] == 'browser':
Any Firefox-on-xulrunner worries here? Seems like there should be.
r=me with the above addressed.
Attachment #8391759 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Addressed all of the comments: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ea1662020a
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 13•11 years ago
|
||
I don't think this really needs a dev-doc, this is not used externally and is just a gross feeds implementation detail.
Keywords: dev-doc-needed
Assignee | ||
Comment 14•11 years ago
|
||
I set the dev-doc-needed because this removes a Web-facing property from window.
Keywords: dev-doc-needed
Assignee | ||
Comment 15•11 years ago
|
||
This broke XULRunner builds: <https://tbpl.mozilla.org/php/getParsedLog.php?id=36495684&tree=Mozilla-Central#error0>
I took out the bit which was supposed to not break xulrunner <http://hg.mozilla.org/integration/mozilla-inbound/rev/c46d3d35b106>. I guess Firefox over XULRunner won't have feed support, or something. CCing Mossop to tell me what I should do, if anything.
Assignee | ||
Comment 16•11 years ago
|
||
Err, actually CCing Mossop.
Comment 17•11 years ago
|
||
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15)
> This broke XULRunner builds:
> <https://tbpl.mozilla.org/php/getParsedLog.php?id=36495684&tree=Mozilla-
> Central#error0>
>
> I took out the bit which was supposed to not break xulrunner
> <http://hg.mozilla.org/integration/mozilla-inbound/rev/c46d3d35b106>. I
> guess Firefox over XULRunner won't have feed support, or something. CCing
> Mossop to tell me what I should do, if anything.
That's probably going to break some linux distro's. I don't know webidl enough to know what the right answer is. Is there a way to have the webidl and related bits be defined in the browser/ tree?
Comment 18•11 years ago
|
||
Can't you just build BrowserFeedWriter.webidl as part of browser/?
Comment 19•11 years ago
|
||
> Is there a way to have the webidl and related bits be defined in the browser/ tree?
Not really, no, because WebIDL is designed for the web platform, which doesn't have concepts like independent pieces or ordering.
We can just move the header to somewhere that's built in xulrunner builds, like content/base or whatnot...
Comment 20•11 years ago
|
||
We should just get rid of the friggin' thing and reimplementing feed support in some other way, really, but that doesn't really help Ehsan at the moment.
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
Sounds to me you could either move FeedWriterEnabled.h to somewhere in toolkit, or add its source path as INCLUDES in dom/bindings. That being said, how does webidl react to an implementation not being there? For example, what happens in the webapp runtime, which doesn't have browser data?
Assignee | ||
Comment 23•11 years ago
|
||
Moved the header to content/base/src and re-enabled this interface for xulrunner:
https://hg.mozilla.org/integration/mozilla-inbound/rev/114fde4642e0
Flags: needinfo?(dtownsend+bugmail)
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #22)
> Sounds to me you could either move FeedWriterEnabled.h to somewhere in
> toolkit, or add its source path as INCLUDES in dom/bindings. That being
> said, how does webidl react to an implementation not being there? For
> example, what happens in the webapp runtime, which doesn't have browser data?
You will get a runtime js exception when trying to call into the API.
Assignee | ||
Comment 25•11 years ago
|
||
Sorry didn't mean to needinfo!
Flags: needinfo?(dtownsend+bugmail)
Comment 26•11 years ago
|
||
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 27•11 years ago
|
||
> 1.2 --- a/browser/components/feeds/src/FeedWriterEnabled.h
> 1.3 +++ /dev/null
> 3.2 --- /dev/null
> 3.3 +++ b/content/base/src/FeedWriterEnabled.h
You should have used hg mv.
Assignee | ||
Comment 29•11 years ago
|
||
(In reply to comment #27)
> > 1.2 --- a/browser/components/feeds/src/FeedWriterEnabled.h
> > 1.3 +++ /dev/null
> > 3.2 --- /dev/null
> > 3.3 +++ b/content/base/src/FeedWriterEnabled.h
>
> You should have used hg mv.
Sorry, I use git locally and sometimes mess things up when extracting a patch out of it to land into hg. Fortunately this file did not have a lot of history.
Comment 30•11 years ago
|
||
(In reply to Masatoshi Kimura from comment #28)
> Isn't SeaMonkey affected by this change?
Not yet; because we're not a listed app the WebIDL doesn't get compiled in. (Although for people building SeaMonkey over XULRunner I'm not sure whether the existing category overrides WebIDL or not.)
Flags: needinfo?(neil)
Comment 31•11 years ago
|
||
> I'm not sure whether the existing category overrides WebIDL or not.
WebIDL bits take precedence over the existing category if both are registered for the same name, as far as I can tell.
Does seamonkey use the same contractid for this name? If so, does it expose the same API?
Updated•11 years ago
|
Flags: needinfo?(neil)
Comment 32•11 years ago
|
||
(In reply to Boris Zbarsky from comment #31)
> Does seamonkey use the same contractid for this name?
Currently it doesn't, but that could be changed.
> If so, does it expose the same API?
It exposes no API at all (except for the limitation that nsIXPCScriptable is itself not scriptable and therefore content can always call QueryInterface(nsIDOMGlobalObjectConstructor) on the object) so that's not a problem here.
Flags: needinfo?(neil)
Comment 33•11 years ago
|
||
> It exposes no API at all
I'm a bit confused, then. What is the purpose of SeaMonkey's BrowserFeedWriter?
Comment 34•11 years ago
|
||
This won't affect content, but for reference:
https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility
Keywords: dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•