Closed Bug 1286131 Opened 3 years ago Closed 3 years ago

Let us specify a URI of memory report to load from in the about:memory URL

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox50 --- affected

People

(Reporter: ting, Assigned: ting)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

If we can let about memory to load the memory report from a URI, like:

  about:memory?uri=https://...

Then we can add such a link on Socorro (bug 1061371), so checking memory report would be a lot easier.
We'd have to be very careful about the security implications.
Bug 859603 is about adding a button to do this. So this bug can be about extending that from the button to the URL.
Depends on: 859603
Attached patch wip v1 (obsolete) — Splinter Review
Simply reuse the code of loading a file to load a URI by NetUtil.newChannel(). Tried and it worked.

Because a file path can also be a URI, I'd like to remove the feature of "?file=", what do you guys think? Also if there're more than two URIs, a diff will be generated.
Thank you for working on this.

> Because a file path can also be a URI, I'd like to remove the feature of
> "?file=", what do you guys think?

So you could use "?uri=" with a local filename? Sounds ok, but beware that the main use of "?file=" was for B2G, which had a script that uploaded memory reports from a device to a computer and then invoked Firefox on a "?file=" URL. So this change would break that. Not sure if that's important now, but it might be safest to allow "?file=" and "?uri=" as synonyms.

> Also if there're more than two URIs, a diff will be generated.

If two files are present, generating a diff is a good idea. If three or more are present, an error message would be best.
(In reply to Nicholas Nethercote [:njn] from comment #4)
> So you could use "?uri=" with a local filename? Sounds ok, but beware that

Yes, for instance: about:memory?uri=file:///home/ting/Desktop/memory-report.json.gz

If there're no other concerns, I prefer to fix the scrip [1] for B2G.

[1] https://github.com/mozilla-b2g/B2G/blob/master/tools/get_about_memory.py#L254
> If there're no other concerns, I prefer to fix the scrip [1] for B2G.

Sounds good.
Assignee: nobody → janus926
Not sure whom should I ask for a security review.
Attachment #8770068 - Attachment is obsolete: true
Attachment #8770788 - Flags: review?(n.nethercote)
Attachment #8770793 - Flags: review?(n.nethercote)
Attachment #8770793 - Flags: review?(n.nethercote) → review+
(In reply to Ting-Yu Chou [:ting] from comment #7)
> Not sure whom should I ask for a security review.

Maybe Dan could give some feedback.
Flags: needinfo?(dveditz)
Comment on attachment 8770788 [details] [diff] [review]
part1 v1: load memory report of the uris in about:memory url

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

I tested this. It works well with file: URLs but I can't get it to work with memory report files on Bugzilla. E.g. these ones:

  about:memory?uri=https://bugzilla.mozilla.org/attachment.cgi?id=8770802   (zipped)
  about:memory?uri=https://bugzilla.mozilla.org/attachment.cgi?id=8770802   (unzipped)

I get "SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data", but when I download those attachments and load them locally they work ok. Any idea what is wrong?

Also, you'll need to update this mochitest: toolkit/components/aboutmemory/tests/test_aboutmemory4.xul

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +9,2 @@
>  //
> +//     about:memory?uri=file:///home/username/reports.json.gz

Maybe include a website example as well?
Attachment #8770788 - Flags: review?(n.nethercote) → feedback+
I'll double check the code parsing URL.
Not only the code parsing URL, determine whether it's a gzipped one or not by the URL is not sufficient...
The "Content-Type" from getResponseHeader() of the opened nsIHttpChannel (https://bugzilla.mozilla.org/attachment.cgi?id=8770802) is "text/html; charset=iso-8859-1", and I couldn't find a way to get "application/x-gzip-compressed" from the redirected target's response.  So my workaround is always try to decompress the received data, if it fails with NS_ERROR_INVALID_CONTENT_ENCODING, then open the URI again but don't decompress this time.

This seems to be stupid, but the code is much easier. I'll fix the test tomorrow.
Also, I've read that about: URLs are unlinkable from web content as of 49, so it won't be possible to link to these from Soccoro. (Though I guess people could copy and paste the URL into the address bar.)
What a bad news for a lazy person... /_\
The test will try both "file://" and "chrome://" URIs. I was hoping it can get the report from http://mochi.test/, but seems that requires mochitest.ini.
Attachment #8770788 - Attachment is obsolete: true
Attachment #8771216 - Flags: review?(n.nethercote)
Comment on attachment 8771216 [details] [diff] [review]
part1 v2: load memory report of the uris in about:memory url

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

This version works with Bugzilla-attached reports. Lovely!

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +676,5 @@
>        onStopRequest: function(aR, aC, aStatusCode) {
>          try {
>            if (!Components.isSuccessCode(aStatusCode)) {
> +            if (aStatusCode == Cr.NS_ERROR_INVALID_CONTENT_ENCODING) {
> +              // The content isn't a gzipped report, try again without conversion.

Change "conversion" to "unzipping"?
Attachment #8771216 - Flags: review?(n.nethercote) → review+
(In reply to Ting-Yu Chou [:ting] from comment #17)
> What a bad news for a lazy person... /_\

It's safer. about: urls can have chrome privileges. I remember about:memory is one of them. I wouldn't want to click some url that maybe crafted by someone with malicious attachment (on bugzilla, for example).
(In reply to Nicholas Nethercote [:njn] from comment #19)
> > +            if (aStatusCode == Cr.NS_ERROR_INVALID_CONTENT_ENCODING) {
> > +              // The content isn't a gzipped report, try again without conversion.
> 
> Change "conversion" to "unzipping"?

Changed to "The content isn't gzipped, try again without unzipping."

Try is closed, I'll try again later.
Attachment #8770793 - Attachment is obsolete: true
Attachment #8771216 - Attachment is obsolete: true
Attachment #8770793 - Attachment is obsolete: false
Anyone else I can ask for a security review?
I just found this: https://wiki.mozilla.org/Security/Reviews/Review_Request_Form

Looks like you need to put set sec-review? on the patch.
Those aren't a thing any more, AFAIK. Maybe Gijs could give some feedback? He knows front end security stuff. Though it looks like he's going on PTO roon.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Andrew McCreight [:mccr8] from comment #25)
> Those aren't a thing any more, AFAIK. Maybe Gijs could give some feedback?
> He knows front end security stuff. Though it looks like he's going on PTO
> roon.

Yeah, unfortunately I don't have a whole lot of time to look at this.

You're right that web content can't link to about: URIs anymore. And that's probably a good thing in this case, so that small mistakes in about:memory don't lead to remotely exploitable issues... I'm a little worried about this change, and especially that there's no list of which protocols you're allowing (so you could use this to force about:memory to talk to a websockets URI, or to a chrome:// URI, or... ). For some examples of how this kind of thing has been abused in the past with about:reader, see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1147597 or bug 778582 or bug 1182778 or bug 1195976. In fact, as written I don't think the patch does anything to prevent e.g. javascript: or data: URIs from executing, which might lead to chrome-level XSS if anybody can find a way of linking to this. I also don't know if you're following either http or meta redirects.

For defense in depth reasons, I'd suggest at least limiting which protocols you accept in the argument to either file or http or https. I'd also want to make sure that a web server couldn't then follow up by redirecting from http: to some other protocol and have the redirect followed.

Really, if the main usecase here is socorro, it sounds like it might make more sense to make socorro serve memory reports with a custom mimetype that we automatically handle with about:memory, or to offer an easy UI on about:memory to just pass it e.g. the UUID of a crashreport and to use an XHR to fetch the memory report associated with the crash, or something like that.
Flags: needinfo?(gijskruitbosch+bugs)
Then maybe limit the URI to file:, https://bugzilla.mozilla.org, and https://crash-stats.mozilla.com. There's another use case of the memory report uploaded to bugzilla.

Does that sound OK?
(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #26)
> Really, if the main usecase here is socorro, it sounds like it might make
> more sense to make socorro serve memory reports with a custom mimetype that
> we automatically handle with about:memory, or to offer an easy UI on

Actually I quite like this mimetype idea, but I don't know how to deal with the memory report on bugzilla.
(In reply to Ting-Yu Chou [:ting] from comment #28)
> (In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #26)
> > Really, if the main usecase here is socorro, it sounds like it might make
> > more sense to make socorro serve memory reports with a custom mimetype that
> > we automatically handle with about:memory, or to offer an easy UI on
> 
> Actually I quite like this mimetype idea, but I don't know how to deal with
> the memory report on bugzilla.

You can specify any attachment mimetype on bugzilla, so that approach would work for bugzilla - you might need to change the mimetype manually, though you could certainly talk to BMO folks to see if they could do something about that. Note that bugzilla serves attachments off a different domain than BMO, so I don't know if only allowing bugzilla.m.o for linking would work.
Bug 1061371 is going to add a link to the memory report in the UI, whether opens it directly in about:memory or not is not that important anyway.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.