685110 - High memory usage on "view selection source"

Status: NEW

(Core :: General, defect)

Description

[marc.brevoort]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0.1) Gecko/20100101 Firefox/6.0.1
Build ID: 20110830185545

Steps to reproduce:

Visit http://www.cisco.com/en/US/docs/security/asa/asa72/system/message/logmsgs.html
Select a full attack description paragraph at the bottom of the page, e.g.

"733105
Error Message    %ASA-4-733105: TD_SYSLOG_TCP_INTERCEPT_BURST_RATE_EXCEED 
Explanation    This message is generated when the system is under Syn flood attacks, if the burst rate exceeds the configured threshold.
Recommended Action    None required. "

Right-click and "View selection source"


Actual results:

Memory usage spiked to 100%, firefox screen greyed out. This was followed by a full system freeze where even the mouse cursor and numlock light would no longer respond. The same actions after a reboot rendered the same result.

Running Ubuntu 10.4 LTS, Firefox 6, encrypted drive.


Expected results:

Memory usage more or less stable; no freeze and being able to view the bit of source previously selected.

Comment 1

[marc.brevoort]

Specifically the release is 6.0.1 - Mozilla Firefox for Ubuntu Canonical - 1.0. I'm currently on the *release* update channel.

Comment 2

[Matthias Versen [:Matti]]

A system freeze is a system bug but I can confirm a massive memory usage (1.5GB+) that leads to a crash on my 32bit system. Someone needs to look where we spend the memory

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, presumably at least part of it is a string encoding all the text on that page... probably multiple times.  :(

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Massif output

Marc, thanks for very precise steps-to-reproduce.

I can reproduce this.  The first time Firefox froze.  The second time it didn't, and the (very large) source started showing and my resident memory usage reached 4GB before I killed it.  (My machine has 16GB of RAM.)

I managed to do a run under Massif.  A partial allocation tree at the peak usage point is attached.  As bz suggests, it does look like it's mostly strings -- 91% of the heap memory allocated is from nsStringBuffer::Alloc.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So looking at that massif output....

  17% is the huge data: URI.
  17% is the huge view-source: URI wrapping the data: URI.

I wonder whether we can do something to allow the latter to share the string data with the former... the problem is that data: URIs do not keep their spec as a single string.

Or maybe we should use some sort of filedata: URI here or something instead of data:?  Is that doable?  In any case, at best that will save us 17% of the memory, or in this case 190MB.

Then we have the 51% that's attribute values.  Looks like those are the href strings for all the various things we now linkify in view-source.  One problem there is that a bunch of those links are of the form <a href="#wp4649442"> or whatnot and the patch for bug 17612 as far as I can tell makes all those into absolute urls using data: URI as the base URI, which makes them all huge, and then we blow through way too much memory.

The right fix is probably to make "view selection source" somehow use the original document URI as the base URI.  We'd still use a good bit of memory for all the absolute urls, but hopefully not O(N^2) in the data size as now.

Henri, is that easier to do in your new view-source impl?

Comment 6

[marc.brevoort]

Thanks to all of you for the great work you're doing on this people, it's much appreciated.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> The right fix is probably to make "view selection source" somehow use the
> original document URI as the base URI.

I think once the code is viewing selection source, it doesn't know that it's viewing selection source as opposed to viewing the source of full data: URL document.

It would be doable to check if the base URL is data: and not to resolved relative URLs against the base if the base is data:.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

The relative URIs are a killer here, though this is certainly a fairly nasty edge-case.  If it was using the original document in some manner then the relative URLs wouldn't be so large.

Hmmm.  If there was some equivalent to data: that put the data into a object like "temp:random_name" instead of directly in the URI itself, then this would all be much easier.  Not a change likely to happen, though.

If data: URIs could reference each other you could collapse the storage requirement (kinda ref-count the data).  Not simple, not a win for anything except relative URIs in a data: document.  Or we could do some sort of late-binding for relative data URLs.

It wouldn't be as large an issue (just some extra mem use) if the edge case wasn't so bad.  Refuse to build relative data: URIs over a certain size?  Ugh.  Refuse to build them at all?  That might break things.. (yes?)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> If there was some equivalent to data: that put the data into a object like
> "temp:random_name" instead of directly in the URI itself, then this would all be much
> easier.  Not a change likely to happen, though.

See mention of filedata in comment 5.

> Or we could do some sort of late-binding for relative data URLs.

That won't help.  The problem here is that view-source generates absolute url strings that it then passes to SetAttr on DOM elements.

If this were just nsIURI objects, we would not have a problem, because that case dholbert and I _did_ think about when doing bug 308590 (which I guess is the other contributing factor here).  See bug 308590 comment 30.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So I think the simplest fix here is probably to nuke the "make all the URIS absolute" code entirely and just set the base URI of the document.

The simplest way to do _that_ for the view partial source case is to just inject a <base> element into the DOM constructed by view-source, I suspect.

The view-source code itself could produce such a <base> element too, I guess, to cover cases when view-source:http://something is used as a string (when it's done via the view-source window the UI could once again inject the <base>.

Thoughts?

Comment 11

[Nicholas Nethercote [inactive]]

I don't understand why selecting a small piece of text in the page results in such an enormous amount of source.  Can someone explain? :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

The selection source shows the smallest amount of source that gives a balanced subtree that contains both ends of the selection.

So if you select some text the <body>, the selection source will show the source of the whole <body> (with the part that you actually had selected highlighted in the source view).

Comment 13

[Henri Sivonen (:hsivonen)]

See bug 698691 for the code that needs tweaking in order to change the View Source base URL.

Comment 14

[Andrew McCreight [:mccr8]]

Downgrading because this is probably a more developer-focused feature that isn't commonly used.