Errors in files with large data URIs can cause the error console to consume gigabytes of memory and lock up the browser

RESOLVED FIXED in mozilla19

Status

Toolkit Graveyard
Error Console
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Trunk
mozilla19
Bug Flags:
in-testsuite +

Details

(Whiteboard: [MemShrink:P3])

Attachments

(2 attachments, 8 obsolete attachments)

4.43 KB, patch
Details | Diff | Splinter Review
5.71 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
If errors are reported in a file with a large data URI, the error console can consume gigabytes of memory and essentially lock up the browser. The root cause is that the "sourceName" of the file with the error is stored in an attribute in the error console's XUL interface, which defeats any string buffer sharing that would otherwise occur and causes the URI to be copied for each error. Even if that weren't a problem, displaying the huge URIs directly in the error console would also be expensive and make the user experience very poor.

The file in attachment 655822 [details] brings Firefox to its knees if the error console is open, even if bug 786108 and bug 795086 are addressed.
(Assignee)

Updated

6 years ago
Assignee: nobody → seth
Whiteboard: [MemShrink:P3]
(Assignee)

Comment 1

6 years ago
Created attachment 666760 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

This resolves the performance and memory issues when combined with the patches for bug 786108 and bug 795086. (They resolve issues in other subsystems that are also triggered by these huge data URI errors.)

The approach taken is to only store a shortened from the URI in the "href" attribute, which is what is displayed to the user visually. (If the URI gets shortened, "..." is appended to to it to make it clear that that has happened.) The full URI is stored as a JavaScript object and the click handler uses that URI to correctly display the source file.

Thoughts? Personally, I'm not too happy about "this.parentNode.parentNode.parentNode.parentNode.sourceName", and I'd appreciate some advice about how to do that part more cleanly. I'm not very familiar with XUL =)
Attachment #666760 - Flags: feedback?
(Assignee)

Updated

6 years ago
Attachment #666760 - Flags: feedback? → feedback?(jaws)
Comment on attachment 666760 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Redirecting to one of the devtools peers for feedback.
Attachment #666760 - Flags: feedback?(jaws) → feedback?(rcampbell)
(Assignee)

Comment 3

6 years ago
Created attachment 666772 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

A much improved version of the same patch. Filtering and copy/paste are now handled correctly, and document.getBindParent(this) replaces the parentNode.parentNode... chain. This one might be ready for review, depending on the feedback received.
Attachment #666760 - Attachment is obsolete: true
Attachment #666760 - Flags: feedback?(rcampbell)
(Assignee)

Updated

6 years ago
Attachment #666772 - Flags: feedback?(rcampbell)
(Assignee)

Comment 4

6 years ago
Created attachment 666778 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Ack; hg typo - last patch was empty. Here's the correct patch.

Going ahead and requesting review.
Attachment #666772 - Attachment is obsolete: true
Attachment #666772 - Flags: feedback?(rcampbell)
Attachment #666778 - Flags: review?(rcampbell)

Comment 5

6 years ago
> +            if (aObject.sourceName.length > 100) {
Optimize for the more frequent case < 101, also why this magic number? what happens between 100 and 101?

> +              row.setAttribute("href", aObject.sourceName.substring(0, 100) + '...');
Please use a real UTF-8 ellipsis … here.
(Assignee)

Comment 6

6 years ago
Created attachment 667063 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Thanks for the feedback, Philip. I've updated the patch based on your suggestions.

I assume your comment about optimizing for the common case was suggesting that I switch the order of the 'then' and 'else' branches of the 'if' statement; let me know if you meant something more than that.

Regarding the magic number, indeed, it is basically arbitrary. The things we are trying to avoid here are (1) massive memory consumption and (2) time spent copying huge strings. We can allow pretty huge URIs from a human perspective and still fix the problem. There are a maximum of 250 rows allowed in the error console at a time, and these are UTF-16 strings, so memory usage is proportional to 250 * 2 * n, where n is the maximum number of characters we store in an attribute directly.  I took a look around the web at the size of URIs we can generally expect users to deal with (see e.g. here: http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url) and concluded that 2048 might be a better limit, since it is the limit imposed by the sitemaps protocol and apparently Googlebot and IE8-9. This is still arbitrary, of course, but it seems it will cover the vast majority of useful cases. I've updated the patch to use 2048 as the limit.
Attachment #666778 - Attachment is obsolete: true
Attachment #666778 - Flags: review?(rcampbell)
Attachment #667063 - Flags: review?(rcampbell)
(Assignee)

Comment 7

6 years ago
Meant to say this above, but with 2048 as a limit we expect to devote ~1 MB of memory to these attributes in the worst case, which isn't too bad. The actual usage is higher due to various sources of waste (unused buffer space in the strings, for example) but in my tests memory usage was acceptable on the test case with an attribute size limit of 2048.

Comment 8

6 years ago
> Meant to say this above, but with 2048 as a limit we expect to devote ~1 MB of memory
> to these attributes in the worst case.

I see thanks. This is only a suggestion (or bikeshed) but instead of looking at the maximum as in (http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url) perhaps we should look at the median instead to cut down the waste. But by all means lets get something checked in first instead of quibbling.
Comment on attachment 667063 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.


+            if (aObject.sourceName.length <= 2048) {

you should declare the magic number in a const.

If the href is included just for display purposes, I don't think you'll need 2048 characters. At the risk of furthering the bikeshed, you could make that something ~80 characters so you can see that you've got a data URI and get the contents out of mSourceName if needed.

you could also check to see if the href starts with a data: scheme and only filter those since those are likely to be the worst-offenders in the long-URI arms race.

I'd also like to see a unittest with this.

Canceling review.
Attachment #667063 - Flags: review?(rcampbell)
(Assignee)

Comment 10

6 years ago
Rob, when you say unit test, do you mean a browser chrome test? If so, where should the test live? I don't see a test directory under toolkit/components/console.

I assume that my goal with the test is to open an error console, load a tab that will generate a huge number of errors with very long sourceNames, and essentially make sure that the load completes. (Since without this bugfix we will OOM or take too long and the test framework will kill the test.) Sound right, or did you have something else in mind?

I'm fine with reducing the displayed URL length. I like Philip's suggestion of a statistical approach. Based upon (http://www.supermind.org/blog/740/average-length-of-a-url-part-2) it looks like we can aim for around 200 characters and hit > 99% of all URLs. I'd rather not filter data: URIs specifically since I want to make sure we fix all cases.
(Assignee)

Comment 11

6 years ago
Created attachment 667698 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Updated version of the patch that stores the source URL length limit in a constant and uses a limit of 200 rather than 2048.

Not flagging for review as the unit test issue still isn't resolved.
Attachment #667063 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
(In reply to Seth Fowler [:seth] from comment #10)
> Rob, when you say unit test, do you mean a browser chrome test? If so, where
> should the test live? I don't see a test directory under
> toolkit/components/console.
> 
> I assume that my goal with the test is to open an error console, load a tab
> that will generate a huge number of errors with very long sourceNames, and
> essentially make sure that the load completes. (Since without this bugfix we
> will OOM or take too long and the test framework will kill the test.) Sound
> right, or did you have something else in mind?

@robcee: Awaiting your feedback on this.
Flags: needinfo?(rcampbell)
that sounds right, except rather than browser chrome, the test should be chrome only as described here:

https://developer.mozilla.org/en-US/docs/Mozilla_automated_testing

"chrome: tests running with high privileges that can test a lot of the browser's functionality.  Tests that verify JavaScript interactions with chrome-level objects should go here. These tests do not exist for mobile Firefox."

There should be some examples of these in toolkit/aboutmemory/tests to crib from.

Feel free to ping in irc in #fx-team, #developers or #devtools if you need help.
Flags: needinfo?(rcampbell)
(Assignee)

Comment 14

6 years ago
Created attachment 670977 [details] [diff] [review]
Chrome test for bug 796179.

@robcee: Thanks! I've now put together a chrome test for this issue.

Putting both the test and the modified patch up for review.
Attachment #670977 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Attachment #667698 - Flags: review?(rcampbell)
Comment on attachment 670977 [details] [diff] [review]
Chrome test for bug 796179.

>+      window.setTimeout(function() {
>+                          // Clean up.
>+                          var elem = document.getElementById('errorConsoleFrame');
>+                          elem.parentNode.removeChild(elem);
>+                          elem = document.getElementById('badSVG');
>+                          elem.parentNode.removeChild(elem);
>+                          elem = null;
>+
>+                          // Finish the test with a pass.
>+                          ok(true, 'Error console rendered OK.');
>+                          SimpleTest.finish();
>+                        }, 0);
>+    }

Drive-by comment: AFAIK, we discourage "setTimeout" in mochitests, because people make incorrect assumptions about its timeout-length being honored, and that leads to random failures.

Instead of setTimeout(..., 0), you can use SimpleTest.executeSoon(), which doesn't have the baggage of a bogus time-duration.
(Assignee)

Comment 16

6 years ago
That's a good point. I was looking for an alternative to setTimeout, but all I could find was postMessage which does not seem to work properly for chrome windows. I'm not sure how I missed executeSoon; I'll switch over to that.
(Assignee)

Comment 17

6 years ago
Created attachment 670985 [details] [diff] [review]
Chrome test for bug 796179.

Switch from setTimeout to executeSoon per dholbert's recommendation.
Attachment #670977 - Attachment is obsolete: true
Attachment #670977 - Flags: review?(rcampbell)
Attachment #670985 - Flags: review?(rcampbell)
(Assignee)

Comment 18

6 years ago
Created attachment 670996 [details] [diff] [review]
2. Chrome test for bug 796179.

Included features from the Mochitest template in the test. (e.g. a link to the bugzilla page)
Attachment #670985 - Attachment is obsolete: true
Attachment #670985 - Flags: review?(rcampbell)
Attachment #670996 - Flags: review?(rcampbell)
Comment on attachment 667698 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

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

I'm going to give this a tentative r+ with a request for an additional review from a toolkit peer.

I'd also like to see these run through try both on mozilla-central and on comm-central because this has the possibility of breaking both.

::: toolkit/components/console/content/consoleBindings.xml
@@ +18,4 @@
>          <xul:vbox class="console-rows" role="console-rows" xbl:inherits="dir=sortOrder"/>
>        </xul:vbox>
>      </content>
>    

trailing spaces here.

@@ +23,5 @@
>        <field name="limit" readonly="true">
>          250
>        </field>
> +      <field name="hrefMaxLength" readonly="true">
> +        <!-- Limit displayed script error source URL length to improve performance / memory usage. -->

could reference the bug in this comment for future generations.

@@ +31,2 @@
>        <field name="_showChromeErrors">-1</field>
>        

whitespace here.

@@ +200,1 @@
>    

and here.

Patch didn't apply cleanly, possibly because of these whitespaces? Might want to generate your patch with hg diff -w to get rid of those (or, just remove them).

@@ +320,5 @@
>        <method name="filterElement">
>          <parameter name="aRow" />
>          <body><![CDATA[
>            let anyMatch = ["msg", "href", "line", "code"].some(function (key) {
>              return (aRow.hasAttribute(key) && 

trailing whitespace on this line.
Attachment #667698 - Flags: review?(rcampbell)
Attachment #667698 - Flags: review?
Attachment #667698 - Flags: review+
Comment on attachment 670996 [details] [diff] [review]
2. Chrome test for bug 796179.

looks good!
Attachment #670996 - Flags: review?(rcampbell) → review+
Whiteboard: [MemShrink:P3] → [MemShrink:P3][needs try runs, waiting for toolkit review]

Comment 21

6 years ago
Try run for b1e96f59bcdf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b1e96f59bcdf
Results (out of 16 total builds):
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rcampbell@mozilla.com-b1e96f59bcdf
Comment on attachment 667698 [details] [diff] [review]
Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

asking Neil for additional review!
Attachment #667698 - Flags: review? → review?(neil)
(Assignee)

Comment 23

6 years ago
Created attachment 671535 [details] [diff] [review]
1. Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

(Carrying over tentative r+ from :robcee)

Only comment/whitespace changes. I created the diff with the -w switch to avoid whitespace issues appearing in the diff. I may be getting confused due to my inexperience with mercurial patch queues, but it looks to me like that whitespace was preexisting, so '-w' seemed like a better choice.

I also updated the comment to reference the bug # as suggested.
Attachment #667698 - Attachment is obsolete: true
Attachment #667698 - Flags: review?(neil)
Attachment #671535 - Flags: review+
(Assignee)

Comment 24

6 years ago
Comment on attachment 671535 [details] [diff] [review]
1. Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Whoops, should've been r?. Fixed.
Attachment #671535 - Flags: review+ → review?(neil)
(Assignee)

Comment 25

6 years ago
Looks like something went wrong (infra failure?) with the try run in comment 21. I pushed another try run at https://tbpl.mozilla.org/?tree=Try&rev=6fca8d6eab99.
(Assignee)

Comment 26

6 years ago
Ack, that one went wrong too. I'm now placing my hopes on https://tbpl.mozilla.org/?tree=Try&rev=6f03cdc18f7e.

Comment 27

6 years ago
Try run for 6fca8d6eab99 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6fca8d6eab99
Results (out of 16 total builds):
    exception: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mfowler@mozilla.com-6fca8d6eab99
(In reply to Philip Chee from comment #5)
> > +              row.setAttribute("href", aObject.sourceName.substring(0, 100) + '...');
> Please use a real UTF-8 ellipsis … here.
Actually we have a complex localised intl.ellipsis pref if you want to get pedantic ;-)
sigh. I kinda noobed up my try pushes there. I ended up killing them.

The real push ended up here and looks fine:

https://tbpl.mozilla.org/?tree=Try&rev=69f01aaa74d6
(Assignee)

Comment 30

6 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #29)
> The real push ended up here and looks fine:

Yeah, mine looks good too so far.
(Assignee)

Updated

6 years ago
Attachment #671535 - Attachment description: Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user. → 1. Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.
(Assignee)

Updated

6 years ago
Attachment #670996 - Attachment description: Chrome test for bug 796179. → 2. Chrome test for bug 796179.
robcee: just to be clear -- it looks like all of the whitespace issues you brought up in comment 19 are in contextual code, not in code that Seth added.  Did you want him to fix those as part of this patch?
(Assignee)

Comment 32

6 years ago
If so IMO it should be in a separate patch, just to make the revision history easier to read.
(In reply to Daniel Holbert [:dholbert] from comment #31)
> robcee: just to be clear -- it looks like all of the whitespace issues you
> brought up in comment 19 are in contextual code, not in code that Seth
> added.  Did you want him to fix those as part of this patch?

yeah, no need to fix those. I'm fine with a followup bug and fix. I'm forcing myself to not care about those. :)
Comment on attachment 671535 [details] [diff] [review]
1. Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Now that you're storing the source name, I think you should switch all uses of has/getAttribute("href") to use mSourceName instead.

>+            if (aObject.sourceName.length <= this.hrefMaxLength) {
>             row.setAttribute("href", aObject.sourceName);
[I assume you'll fix the indentation here.]

>           let anyMatch = ["msg", "href", "line", "code"].some(function (key) {
>             return (aRow.hasAttribute(key) && 
>                     this.stringMatchesFilters(aRow.getAttribute(key), this.mFilter));
>-          }, this);
>+          }, this) || (aRow.hasAttribute("href") &&
>+                       this.stringMatchesFilters(aRow.mSourceName, this.mFilter));
You're actually checking the href twice here.

>       <xul:label class="text-link" xbl:inherits="href,value=href" crop="right"/>
[We probably don't need to inherit the href on its own any more, just the value=href.]
Attachment #671535 - Flags: review?(neil) → review-
(Assignee)

Comment 35

6 years ago
Created attachment 679849 [details] [diff] [review]
1. Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Thanks for the feedback Neil. I believe I've addressed your concerns in this updated version of the patch.
Attachment #671535 - Attachment is obsolete: true
Attachment #679849 - Flags: review?(neil)

Updated

6 years ago
Attachment #679849 - Flags: review?(neil) → review+
(Assignee)

Comment 36

6 years ago
Thanks Neil. This just needs a try run and it's good to go. Started one here: https://tbpl.mozilla.org/?tree=Try&rev=b16e715351b2

Comment 37

6 years ago
Try run for b16e715351b2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b16e715351b2
Results (out of 257 total builds):
    success: 235
    warnings: 21
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mfowler@mozilla.com-b16e715351b2
(Assignee)

Comment 38

6 years ago
Looks good. Requesting checkin.
Keywords: checkin-needed
Whiteboard: [MemShrink:P3][needs try runs, waiting for toolkit review] → [MemShrink:P3]
https://hg.mozilla.org/mozilla-central/rev/a557ce61d622
https://hg.mozilla.org/mozilla-central/rev/fc53121e1685
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.