1237908 - pdf viewer needs to use the proper user context id

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Dave Huseby [:huseby]]

in the file browser/extensions/pdfjs/content/PdfStreamConverter.jsm there's this code:

> 1023     var uri = NetUtil.newURI(PDF_VIEWER_WEB_PAGE, null, null);
> 1024     var resourcePrincipal;
> 1025     resourcePrincipal = ssm.createCodebasePrincipal(uri, {});

We have the channel in this context so we should probably take the user context id from the necko origin attributes associated with the channel.

Comment 1

[Dave Huseby [:huseby]]

Attachment: Bug_1237908.patch

this makes sure the origin attributes are used in the resource principal so that isolation remains consistent.  if the pdf is loaded in a user context, then all of the resources pdfjs loads needs to be loaded in that context as well for caching/storage isolation.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 8709204 [details] [diff] [review]
Bug_1237908.patch

I don't know the PDF.js code at all. FAIK it's canonical source it Github (https://github.com/mozilla/pdf.js), maybe this needs to be a pull request over there? Over to Brendan.

Comment 3

[Brendan Dahl [:bdahl]]

I can upstream the code once it lands here. Why was the review cancelled?

Comment 4

[Dave Huseby [:huseby]]

Comment on attachment 8709204 [details] [diff] [review]
Bug_1237908.patch

Hi Jonas, I reviewed this patch in the context of the new way of doing things and this still looks right to me.

Comment 5

[Dave Huseby [:huseby]]

Comment on attachment 8709204 [details] [diff] [review]
Bug_1237908.patch

this one is good to go.

Comment 6

[Dave Huseby [:huseby]]

Attachment: Bug_1237908.patch

updated per feedback.

Comment 7

[Dave Huseby [:huseby]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa9882834461

Comment 8

[Dave Huseby [:huseby]]

lots of red in windows 7.  try one more time with newer code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73cf142cc647

Comment 9

[Dave Huseby [:huseby]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f3c16c37dc

Comment 10

[Dave Huseby [:huseby]]

looks clean, ready to land.

Comment 11

[Dave Huseby [:huseby]]

Attachment: Bug_1237908.patch

r+ by :sicking, updating commit message to be more verbose.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dave Huseby [:huseby] from comment #11)
> Created attachment 8733138 [details] [diff] [review]
> Bug_1237908.patch
> 
> r+ by :sicking, updating commit message to be more verbose.

This looks like the wrong patch. Also, could we just land this upstream and let it land via the usual pdf.js update process (which occurs on a roughly weekly basis)?

Comment 13

[Dave Huseby [:huseby]]

Attachment: Bug_1237908.patch

this is the correct patch.  it is already r+ by :sicking.

I'm not sure how to upstream this patch?  Where is the pdf.js repo?

Comment 14

[Dave Huseby [:huseby]]

updated

Comment 15

[Yury Delendik (:yury)]

Dave, the repo/file is at https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/PdfStreamConverter.jsm#L1081

Comment 16

[Dave Huseby [:huseby]]

Attachment: Make pdf.js use the correct principal origin attributes when loading subresources.

already r+ by :sicking.  this is a github PR to upstream this patch into the main pdf.js repo.

Comment 17

[Dave Huseby [:huseby]]

merged upstream: https://github.com/mozilla/pdf.js/commit/f9a0dc1188745a6fcce5a50e1018df8decd9124d