Closed Bug 739803 Opened 12 years ago Closed 12 years ago

SPDY indicator add-on causing memory leak

Categories

(WebExtensions :: General, defect)

Product:
WebExtensions
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: boaz.dodin, Unassigned)

References

(
URL
)

Details

(Whiteboard: [MemShrink:P3]) 

As per this issue on the add-on's development GitHub, there is a memory leak confirmed by the developer:
https://github.com/chengsun/moz-spdy-indicator/issues/4
Blocks: LeakyAddons
Whiteboard: MemShrink
This add-on is on AMO.
(In reply to Justin Lebar [:jlebar] from comment #1)
> This add-on is on AMO.

Yes, an AMO url of the add-on is at "URL" field of this bug...
(In reply to Boaz Dodin from comment #2)
> (In reply to Justin Lebar [:jlebar] from comment #1)
> > This add-on is on AMO.
> 
> Yes, an AMO url of the add-on is at "URL" field of this bug...

Yeah, I saw after I commented.  :)  But in any case, I wanted to make sure others knew / noticed.
Whiteboard: MemShrink → [MemShrink]
Confirming leak.  Developer contacted via info request inviting them to cc themselves onto this bug for tracking.
Status: UNCONFIRMED → NEW
Ever confirmed: true
P3 per Memshrink
Whiteboard: [MemShrink] → [MemShrink:P3]
Component: Untriaged → Add-ons
Product: Firefox → Tech Evangelism
Version: 11 Branch → unspecified
According this developer's comment (from yesterday):
https://github.com/chengsun/moz-spdy-indicator/issues/7#issuecomment-4984536
he aware of the memory problem but not going to upload an updated version "for a while".

Is it a time for downgrading the add-on to preliminary review?
Give me(In reply to Boaz Dodin from comment #6)
> According this developer's comment (from yesterday):
> https://github.com/chengsun/moz-spdy-indicator/issues/7#issuecomment-4984536
> he aware of the memory problem but not going to upload an updated version
> "for a while".
> 
> Is it a time for downgrading the add-on to preliminary review?

I'll look into it and try to find a way to fix it, give me one or two days.
cheng: is there any update on your fix you can give us?
(In reply to Andrew Williamson [:eviljeff] from comment #8)
> cheng: is there any update on your fix you can give us?

The memory leak itself is not *too* serious, (I hope). The add-on basically saves the unique URLs of every request made, inside each tab, but that associative array is never cleared out. I save URLs because I don't know how to find a tab's top-level URI when I observe the first page request through an http-on-examine-response observer (every way that I've tried returns "about:blank" because it's observed too early in the page load process), so instead I save all the URLs and process it on the tab's load/pageshow event.

I've written some code that clears out the array every tab load, but that may continue to use up memory if a page is never loaded and XHRs are being sent out to different URIs in the background. Plus, it means that the SPDY indicator's state will be misleading until the page has finished loading.

Ideally there's some way that I've missed to find the tab's URI whilst it's still loading?
> The memory leak itself is not *too* serious, (I hope). The add-on basically saves the unique URLs of 
> every request made, inside each tab, but that associative array is never cleared out.

The leak being observed here is a zombie compartment, which means that the entire window, and all its JS and DOM objects, is being kept alive.  You said it's an associative array -- if the URLs are the key/value, what's the other thing stored in the array?

If it was just the URLs, we probably never would have noticed the issue.  Although since we're talking about it, saving all URLs will eat up memory over time too -- they can be pretty big sometimes!

> I save URLs because I don't know how to find a tab's top-level URI when I observe the first page 
> request through an http-on-examine-response observer

I'm not sure what you're trying to do, but if you just want to know the top-level URI, you could register a webprogresslistener on the top-level docshell and listen for locationchange events.  See nsIWebProgress / nsIWebProgressListener.

See e.g. browser/base/content/test/browser_bug623155.js, or mobile/xul/chrome/content/bindings/browser.js.
(In reply to Justin Lebar [:jlebar] from comment #10)
> > The memory leak itself is not *too* serious, (I hope). The add-on basically saves the unique URLs of 
> > every request made, inside each tab, but that associative array is never cleared out.
> 
> The leak being observed here is a zombie compartment, which means that the
> entire window, and all its JS and DOM objects, is being kept alive.  You
> said it's an associative array -- if the URLs are the key/value, what's the
> other thing stored in the array?

What. It seems we're talking about two different issues. I have never noticed zombie compartments with my addon. Could you please try to reproduce with this more up-to-date version: https://github.com/downloads/chengsun/moz-spdy-indicator/SPDYRestartless.xpi

The associative-ness is just to ensure that I don't store the same URL twice, the key is the URL and the value is simply true.

> 
> If it was just the URLs, we probably never would have noticed the issue. 
> Although since we're talking about it, saving all URLs will eat up memory
> over time too -- they can be pretty big sometimes!
> 
> > I save URLs because I don't know how to find a tab's top-level URI when I observe the first page 
> > request through an http-on-examine-response observer
> 
> I'm not sure what you're trying to do, but if you just want to know the
> top-level URI, you could register a webprogresslistener on the top-level
> docshell and listen for locationchange events.  See nsIWebProgress /
> nsIWebProgressListener.
> 
> See e.g. browser/base/content/test/browser_bug623155.js, or
> mobile/xul/chrome/content/bindings/browser.js.

OK, I'll look into that. Thanks.
> I have never noticed zombie compartments with my addon.

Oh, you're right!  I'm sorry.  You have the distinction of being one of the very few non-zombie add-on leaks (that we know of, anyway).
(In reply to Justin Lebar [:jlebar] from comment #12)
> > I have never noticed zombie compartments with my addon.
> 
> Oh, you're right!  I'm sorry.  You have the distinction of being one of the
> very few non-zombie add-on leaks (that we know of, anyway).

And that's a good thing? :)

(In reply to Justin Lebar [:jlebar] from comment #10) 
> you could register a webprogresslistener on the top-level
> docshell and listen for locationchange events.  See nsIWebProgress /
> nsIWebProgressListener.

Close, but the WebProgressListener fires after the observer; not what I want.

Basically from the observer I check for the injected X-Firefox-Spdy header, and from there I need to figure out whether the request was for the top-level document or whether it came from e.g. an embedded script. Thus if the WebProgressListener doesn't fire beforehand to tell me what the top-level document's URI is, I need to store a list of all URI's that have been requested by SPDY between the time that the observer and the WebProgressListener fires.

Alternatively I could make the assumption (which seems to be true from observation) that the WebProgressListener always fires immediately after we observe the top-level document's headers but before we observe any other embedded requests, but that seems rather flaky.
Never mind. I got it. Expect a new version to be uploaded to AMO tonight, fixing this.
WebProgressListener doesn't seem to work as well as the straightforward method. The results are far less accurate than previously, especially with Google, which uses various silly hacks to implement features such as instant search.

Here's a compromise which I rather like: if I only store the prePath section of every request,  although this has the potential for memory growth, it is insignificant if I only store unique prePaths -- much less than before (where I was storing the whole URI), and it is also more accurate than the WebProgressListener method. I'll implement this if you guys have no objections to it.
When you visit a large site (e.g. Facebook, CNN), it makes requests from many, many different origins.  So although this would be an improvement, I wouldn't be happy with it.

There should be no reason for any kind of unbounded memory growth here, particularly for a simple add-on like this one.

If I understand your problem correctly, you get a channel in http-on-examine-response, and you want to determine if it's associated with a top-level document load?

How about we look at the load flags?  nsIChannel::LOAD_INITIAL_DOCUMENT_URI looks like it might work.  That might not work with redirects; you may have to play with it.

Sorry to send you around in circles; I'm definitely not an expert here.
Eh. The number of domains is rather bounded, we're talking tens of kilobytes to store them rather than megabytes.

Anyway, I'm not around from tomorrow and I really want to push a new version out on AMO that's already been delayed a while -- which adds restartlessness. Plus, the Google mess that I was talking about means that observers don't work perfectly -- some pages get requested in weird ways and others don't even trigger a request at all. Seeing as Google is *the* main user of SPDY, getting this right is a priority.

Nonetheless, I will have a look at this when I get back on Monday.
> I've written some code that clears out the array every tab load

Does the new proposed version include this code?  If so, I'd be OK with it, if you promise to fix it for real within a reasonable period of time.
(In reply to Justin Lebar [:jlebar] from comment #18)
> > I've written some code that clears out the array every tab load
> 
> Does the new proposed version include this code?  If so, I'd be OK with it,
> if you promise to fix it for real within a reasonable period of time.

No, it gives incorrect results for Google.

(In reply to Justin Lebar [:jlebar] from comment #16)
> When you visit a large site (e.g. Facebook, CNN), it makes requests from
> many, many different origins.

I've made some measurements. HTTPS Facebook results in *no* items added to the array (SPDY not supported), and CNN does not seem to support HTTPS at all, meaning that nothing is added either. (In case I haven't made it clear previously, only SPDY requests are eligible to go into the array)

After some heavy usage of Google search I managed to get five items added to the array, totalling 121 characters. In a separate tab, watching several YouTube videos resulted in 10 strings added to the array, totalling 216 characters. Each array is confined to a tab, and closing the tab results in the destruction of that array. Obviously theoretically speaking the memory growth is unbounded, but practically storing unique prePaths is perfectly fine, at least for now whilst SPDY adoption is relatively limited.

/me away until Monday, I'll get back to this then.
Cheng, do you have any updates on the leak problem?
No.

If unboundedness is a major concern, I could make it remember only the last 10 prePaths, thereby putting an upper limit on the memory usage, but I really don't think this is necessary; see comment 19.
> If unboundedness is a major concern, I could make it remember only the last 10 prePaths, thereby 
> putting an upper limit on the memory usage

Or even 100 prePaths per tab, if you wanted.

I am uncomfortable with unbounded memory usage of any kind.  It often comes back to bite us in edge cases (in this case, the guy who never restarts his browser and never closes any tabs).
(In reply to Justin Lebar [:jlebar] from comment #22)
> > If unboundedness is a major concern, I could make it remember only the last 10 prePaths, thereby 
> > putting an upper limit on the memory usage
> 
> Or even 100 prePaths per tab, if you wanted.

Done.
https://github.com/chengsun/moz-spdy-indicator/commit/32a971fb6d72c1ed178af0b364f1da3550ac4db9

This fix is included in v2.1, which is currently pending review on AMO.
Thanks a lot, Cheng.
The update has been approved on AMO. Is there anything else that needs to be done before closing this bug?
I wish we didn't have this cludge, and I am not convinced it's necessary, but we've tightly bounded memory growth, which was all that this bug was about.  I'm happy to close it.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.