Closed Bug 1237908 Opened 8 years ago Closed 8 years ago

pdf viewer needs to use the proper user context id

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: huseby, Assigned: huseby)

References

Details

(Whiteboard: [userContextId][pdfjs-c-integration][OA])

Attachments

(1 file, 4 obsolete files)

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.
Whiteboard: [userContextId]
Whiteboard: [userContextId] → [userContextId][pdfjs-c-integration]
Attached patch Bug_1237908.patch (obsolete) — Splinter Review
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.
Attachment #8709204 - Flags: review?(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.
Attachment #8709204 - Flags: review?(dolske) → review?(bdahl)
Attachment #8709204 - Flags: review?(bdahl)
I can upstream the code once it lands here. Why was the review cancelled?
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.
Attachment #8709204 - Flags: review?(jonas)
Attachment #8709204 - Flags: review?(jonas)
Comment on attachment 8709204 [details] [diff] [review]
Bug_1237908.patch

this one is good to go.
Attachment #8709204 - Flags: review?(jonas)
Attached patch Bug_1237908.patch (obsolete) — Splinter Review
updated per feedback.
Attachment #8709204 - Attachment is obsolete: true
Attachment #8709204 - Flags: review?(jonas)
Attachment #8722116 - Flags: review?(jonas)
lots of red in windows 7.  try one more time with newer code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73cf142cc647
looks clean, ready to land.
Keywords: checkin-needed
Attached patch Bug_1237908.patch (obsolete) — Splinter Review
r+ by :sicking, updating commit message to be more verbose.
Attachment #8722116 - Attachment is obsolete: true
Attachment #8733138 - Flags: review+
(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)?
Flags: needinfo?(huseby)
Keywords: checkin-needed
Attached patch Bug_1237908.patch (obsolete) — Splinter Review
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?
Attachment #8733138 - Attachment is obsolete: true
Flags: needinfo?(huseby)
Attachment #8733630 - Flags: review+
updated
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
already r+ by :sicking.  this is a github PR to upstream this patch into the main pdf.js repo.
Attachment #8733630 - Attachment is obsolete: true
Attachment #8734949 - Flags: review+
merged upstream: https://github.com/mozilla/pdf.js/commit/f9a0dc1188745a6fcce5a50e1018df8decd9124d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 1263067
Whiteboard: [userContextId][pdfjs-c-integration] → [userContextId][pdfjs-c-integration][OA]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: