Closed
Bug 797890
Opened 8 years ago
Closed 8 years ago
Add button to load memory reports from the clipboard
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file)
7.97 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
This is pretty useful to me in practice!
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #668009 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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 :)
Attachment #668009 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 4•8 years ago
|
||
> 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•8 years ago
|
||
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•8 years ago
|
||
> Is the input here properly sanitized for realz?
Yes: it goes through JSON.parse.
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
> 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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa211d740d88
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•