797890 - Add button to load memory reports from the clipboard

Status: RESOLVED FIXED

Description

[Justin Lebar (not reading bugmail)]

This is pretty useful to me in practice!

Comment 1

[Justin Lebar (not reading bugmail)]

Created attachment 668009 [details] [diff] [review]
Patch, v1

Comment 2

[Justin Lebar (not reading bugmail)]

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]]

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)]

> 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]]

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]]

> Is the input here properly sanitized for realz?

Yes:  it goes through JSON.parse.

Comment 7

[Justin Lebar (not reading bugmail)]

(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)]

> 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]]

https://hg.mozilla.org/mozilla-central/rev/aa211d740d88