Update remaining callsites to use newChannel2 in browser/devtools

RESOLVED FIXED in Firefox 39

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 1115193
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8571541 [details] [diff] [review]
bug_1138641_browser_devtools.patch
Attachment #8571541 - Flags: review?(vporof)
(Assignee)

Updated

3 years ago
Summary: Updated remaining callsites to use newChannel2 in browser/devtools → Update remaining callsites to use newChannel2 in browser/devtools

Comment 2

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8571541 [details] [diff] [review]
> bug_1138641_browser_devtools.patch

Hi Victor!  Hopefully you can find the loadingDocument's associated with each of these call sites that we can use instead of system Principal (like you did in Bug 1115193).  Thanks!
Comment on attachment 8571541 [details] [diff] [review]
bug_1138641_browser_devtools.patch

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

::: browser/devtools/performance/modules/io.js
@@ +86,1 @@
>      channel.contentType = "text/plain";

The document here is the tool's iframe window. Could add it as an extra argument for the `loadRecordingFromFile` method.

I still don't understand what's the point of all of this? I could just as easily say here that "it's ok not to pass a document, because this is an utility function that can be accessed from anywhere", but it's just as valid to say "however, in every single case we have access to a document, that's always chrome".

Seems to me like the rationale between these arguments is very vague, at best.

So which should it be?

::: browser/devtools/shared/theme.js
@@ +23,5 @@
>  
>  /**
>   * Returns a string of the file found at URI
>   */
>  function readURI (uri) {

This method is only used in this file. But same idea applies as for the previous comment: if we tried really hard, of course we could pass in a document. Is it necessary?
Attachment #8571541 - Flags: review?(vporof)
The only reason that I can think of to hunt down the "right" document and passing in as loading node is if/when we enable chrome documents to opt in to security policies. Then I figuring out what policy to use would likely be done through the aLoadingNode argument.

That said, we can probably apply many policies without finding the right document for every network load.
The other thing that finding the right document probably could do in the future, is to show all loads belonging to that document in the developer tools when debugging the chrome document.
Talked with Tanvi. Given that the benefits of passing in a document are in the "nice to have" and "might be good in the future" categories, it would make sense to pass in a document if we have one easily available. Otherwise simply pass in the system principal as the loadingPrincipal.
(Assignee)

Comment 7

3 years ago
Tanvi, I agree it would be great (or at least nice) to have a document; at the moment however I don't think a document is easily available. Besides it seems cumbersome to go through the callsites of loadRecordingFromFile [1] where even there a document might not be that easily accessible. In turn again, changing all those callsites again seems like too much of a trouble compared to the benefit for having a document in devtools (which is chrome anyway).

Still any objections to land this patch as is?

[1] http://mxr.mozilla.org/mozilla-central/search?string=loadRecordingFromFile
Flags: needinfo?(tanvi)
For browser/devtools/performance/modules/io.js, it's easy to access a document, even though it doesn't exist in that particular file. Just find the callers of that method, and pass in `document` as an extra argument. On the other hand, browser/devtools/shared/theme.js is probably fine without a document.
To be more specific, `loadRecordingFromFile` is used in browser/devtools/performance/modules/recording-model.js for the `RecordingModel.prototype.importRecording` method, which in turn is used in browser/devtools/performance/performance-controller.js for `PerformanceController.prototype.importRecording`, which is included in performance.xul, so a document is available.

There's a few tests using this method too.

Comment 10

3 years ago
Let's go ahead with the current patch that uses systemPrincipal.  Victor, if you think it is handy to have the loadingNode for devtools (maybe for when we apply CSP to chrome pages or maybe for something internally in devtools where a loadingNode that indicates which devtool document is being used would be helpful), then please file a bug to add the document parameter to the loadRecrodingFromFile() method and update the loadingNode passed in.

Sound good?
Flags: needinfo?(tanvi)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8571541 [details] [diff] [review]
bug_1138641_browser_devtools.patch

We still need Victor to r+ the patch though - reflagging him for review.
Attachment #8571541 - Flags: review?(vporof)
Attachment #8571541 - Flags: review?(vporof) → review+
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4958eece94f0
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/4958eece94f0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.