Closed Bug 1233899 Opened 8 years ago Closed 8 years ago

fix the feeds converter to use default user context origin attributes

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
49.2 - May 23
Tracking Status
firefox50 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

Details

(Whiteboard: [userContextId][OA][uplift49-])

Attachments

(1 file, 5 obsolete files)

This bug covers fixing the call to createCodebasePrincipal in browser/components/feeds/FeedConverter.js to use a default user context origin attribute.
Attached patch Bug_1233899.patch (obsolete) — Splinter Review
this forces the feed converter to use a default user context origin attribute when it generates the about:feeds page since we're not going to isolate subscribed feeds by user context.
Attachment #8700249 - Flags: review?(jonas)
The loadInfo.originAttributes used when creating the channel for loading the feed document may also need to have the default user context set in its origin attributes for caching reasons.
Component: Security → DOM
Product: Firefox → Core
Whiteboard: [userContextId]
Attached patch Bug_1233899.patch (obsolete) — Splinter Review
rebased and updated
Attachment #8700249 - Attachment is obsolete: true
Attachment #8700249 - Flags: review?(jonas)
Attachment #8709212 - Flags: review?(jonas)
Attachment #8709212 - Flags: review?(jonas)
Attached patch Bug_1233899.patch (obsolete) — Splinter Review
OK, so I dug deeper into this to better understand what is happening and have decided that the solution is to pass the origin attributes along with the default user context id since it is loading the about:feeds page.

We decided that about:feeds would not be isolated by user context for now since the about:feeds page is the only UI for managing feed subscriptions.  Until changes so that it can show the subscriptions broken out into each user context, we'll just force all feed subscriptions to be associated with the default user context.
Attachment #8709212 - Attachment is obsolete: true
Attachment #8713820 - Flags: review?(jonas)
Attachment #8713820 - Flags: review?(jonas)
Attached patch Bug_1233899.patch (obsolete) — Splinter Review
updated to new API
Attachment #8713820 - Attachment is obsolete: true
Attachment #8722048 - Flags: review?(jonas)
Comment on attachment 8722048 [details] [diff] [review]
Bug_1233899.patch

Review of attachment 8722048 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/feeds/FeedConverter.js
@@ +255,5 @@
>          let aboutFeedsURI = ios.newURI("about:feeds", null, null);
>          chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo);
>          chromeChannel.originalURI = result.uri;
> +        let attrs = ChromeUtils.originAttributesFromDict(loadInfo.originAttributes);
> +        attrs.userContextId = 0

Based on our previous conversation, I suspect this isn't needed. And possibly not even desired.

If this code is used to render the UI when a user opens a feed inside a tab, then likely we should honor user contexts in that tab.

If this code is used to parse feeds for use in live bookmarks, then loadInfo.originAttributes.userContextId should already be 0.
Comment on attachment 8722048 [details] [diff] [review]
Bug_1233899.patch

Review of attachment 8722048 [details] [diff] [review]:
-----------------------------------------------------------------

Removing review request given previous comment.
Attachment #8722048 - Flags: review?(jonas)
Whiteboard: [userContextId] → [userContextId][OA]
We just need to do what comment 6 says and figure out if the code is used for the UI or if it is used in live bookmarks.  Once we know that, we will know if it is necessary or not.
Assignee: huseby → nobody
Status: ASSIGNED → NEW
Assignee: nobody → huseby
Status: NEW → ASSIGNED
Priority: -- → P1
Iteration: --- → 49.2 - May 23
Blocks: 1276412
Attached patch Bug_1233899.patch (obsolete) — Splinter Review
The FeedConverter code that creates a principal does so when we convert the loaded feed into a result that can be loaded in the Feed rendering page.  We want to carry the origin attributes from the channel that loaded the original feed document so that the feed rendering page is using a principal with the same origin attributes and therefore caches.
Attachment #8722048 - Attachment is obsolete: true
Attachment #8760037 - Flags: review?(amarchesini)
Comment on attachment 8760037 [details] [diff] [review]
Bug_1233899.patch

Review of attachment 8760037 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/feeds/FeedConverter.js
@@ +260,3 @@
>          chromeChannel.owner =
> +          Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI,
> +                                                    loadInfo.originAttributes);

indentation.
Attachment #8760037 - Flags: review?(amarchesini) → review+
already r+ by :baku, just fixing the indentation mistake he caught.
Attachment #8760037 - Attachment is obsolete: true
Attachment #8760449 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e052220dd9e
fix the feeds converter to use default user context origin attributes. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e052220dd9e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1277293
Whiteboard: [userContextId][OA] → [userContextId][OA][uplift49?]
Whiteboard: [userContextId][OA][uplift49?] → [userContextId][OA][uplift49-]
Blocks: 1329401
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: