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

Status: RESOLVED FIXED

(Toolkit Graveyard :: Error Console, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: 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 =)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 5

[Philip Chee]

> +            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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

[Philip Chee]

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

[Rob Campbell [:rc] (:robcee)]

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.

Comment 10

[Seth Fowler [:seth] [:s2h]]

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.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 13

[Rob Campbell [:rc] (:robcee)]

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.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 15

[Daniel Holbert [:dholbert]]

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.

Comment 16

[Seth Fowler [:seth] [:s2h]]

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.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: Chrome test for bug 796179.

Switch from setTimeout to executeSoon per dholbert's recommendation.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: 2. Chrome test for bug 796179.

Included features from the Mochitest template in the test. (e.g. a link to the bugzilla page)

Comment 19

[Rob Campbell [:rc] (:robcee)]

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.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 670996 [details] [diff] [review]
2. Chrome test for bug 796179.

looks good!

Comment 21

[Mozilla RelEng Bot]

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/[e-mail]-b1e96f59bcdf

Comment 22

[Rob Campbell [:rc] (:robcee)]

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!

Comment 23

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 24

[Seth Fowler [:seth] [:s2h]]

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.

Comment 25

[Seth Fowler [:seth] [:s2h]]

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.

Comment 26

[Seth Fowler [:seth] [:s2h]]

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

Comment 27

[Mozilla RelEng Bot]

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/[e-mail]-6fca8d6eab99

Comment 28

[[e-mail]]

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

Comment 29

[Rob Campbell [:rc] (:robcee)]

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

Comment 30

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 31

[Daniel Holbert [:dholbert]]

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?

Comment 32

[Seth Fowler [:seth] [:s2h]]

If so IMO it should be in a separate patch, just to make the revision history easier to read.

Comment 33

[Rob Campbell [:rc] (:robcee)]

(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 34

[[e-mail]]

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

Comment 35

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 36

[Seth Fowler [:seth] [:s2h]]

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

[Mozilla RelEng Bot]

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/[e-mail]-b16e715351b2

Comment 38

[Seth Fowler [:seth] [:s2h]]

Looks good. Requesting checkin.

Comment 39

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a557ce61d622
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc53121e1685

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a557ce61d622
https://hg.mozilla.org/mozilla-central/rev/fc53121e1685