Last Comment Bug 797890 - Add button to load memory reports from the clipboard
: Add button to load memory reports from the clipboard
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Justin Lebar (not reading bugmail)
:
: Nicholas Nethercote [:njn]
Mentors:
Depends on: 768470
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-04 08:58 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-10-06 12:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (7.97 KB, patch)
2012-10-04 08:59 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-10-04 08:58:55 PDT
This is pretty useful to me in practice!
Comment 1 Justin Lebar (not reading bugmail) 2012-10-04 08:59:22 PDT
Created attachment 668009 [details] [diff] [review]
Patch, v1
Comment 2 Justin Lebar (not reading bugmail) 2012-10-04 09:00:30 PDT
I didn't add a test because it seems like way more effort than it's worth (particularly because we're re-using most of the existing show-from-files code), but I'm happy to figure out a test if you think it's worthwhile.
Comment 3 Nicholas Nethercote [:njn] 2012-10-04 16:11:11 PDT
Comment on attachment 668009 [details] [diff] [review]
Patch, v1

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

I agree a test isn't necessary.

What's the specific use case, BTW?  I'm having trouble imagining it.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +443,4 @@
>      reader.readAsText(aFile);
>  
>    } catch (ex) {
> +    let body = clearBody();

Would it be better to move this to the top of the try block (where it was) and then pass |body| to updateAboutMemoryFromJSONString()?  Not sure, I'll let you decide.

@@ +457,5 @@
> +{
> +  // Get the clipboard's contents.
> +  let cb = Cc["@mozilla.org/widget/clipboard;1"]
> +             .getService(Components.interfaces.nsIClipboard);
> +  let xferable = Cc["@mozilla.org/widget/transferable;1"]

|xferable| strikes me as a very un-jlebar-ish variable name, but I'll allow it :)
Comment 4 Justin Lebar (not reading bugmail) 2012-10-04 16:15:48 PDT
> What's the specific use case, BTW?  I'm having trouble imagining it.

e.g. cjones posts a memory report dump in bug 797189.  I can very quickly open it up, copy it to my clipboard, and click the button in about:memory.  Saving it to a file and then picking the file is much slower.

> |xferable| strikes me as a very un-jlebar-ish variable name, but I'll allow it :)

Wow, was it that obvious I copied this code??
Comment 5 Andrew McCreight [:mccr8] 2012-10-04 16:22:47 PDT
Is the input here properly sanitized for realz? The easier we make it to input random stuff into about:memory the closer we are to some kind of XSS thing. Now, admittedly this still requires the user cutting and pasting, but it is better to be cautious.
Comment 6 Nicholas Nethercote [:njn] 2012-10-04 17:19:42 PDT
> Is the input here properly sanitized for realz?

Yes:  it goes through JSON.parse.
Comment 7 Justin Lebar (not reading bugmail) 2012-10-04 17:22:20 PDT
(In reply to Nicholas Nethercote [:njn] from comment #6)
> > Is the input here properly sanitized for realz?
> 
> Yes:  it goes through JSON.parse.

And we never set innerHTML, so we're good in theory.

It strikes me that for the use-case I'm interested in, being able to specify a URL to fetch the about:memory dump from would be particularly convenient.  That way, we could fetch a compressed dump, whereas right now, if I want to be able to use this convenient copy/paste thing, I have to post uncompressed dumps to bugzilla, which are obscenely large.  But that's for a separate bug.
Comment 8 Justin Lebar (not reading bugmail) 2012-10-05 07:49:09 PDT
> Would it be better to move this to the top of the try block (where it was) and then pass |body| to 
> updateAboutMemoryFromJSONString()?  Not sure, I'll let you decide.

I was on the fence about this, but I think it's maybe better how it is because functions with fewer arguments are better, and functions with fewer assumptions are better.  (Otherwise updateAboutMemoryFromJSONString would assume that the |body| it got had just been cleared.)
Comment 9 Ed Morley [:emorley] 2012-10-06 12:44:22 PDT
https://hg.mozilla.org/mozilla-central/rev/aa211d740d88

Note You need to log in before you can comment on or make changes to this bug.