Closed Bug 1247548 Opened 8 years ago Closed 8 years ago

nsCookieService uses a huge amount of stack space

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: roc, Assigned: steven.low, Mentored)

Details

(Whiteboard: [lang=c++][good first bug][necko-would-take])

Attachments

(1 file)

In a few places we create stack or heap arrays of kMaxNumberOfCookies CookieDomainTuples, which is 3000 * 80 = 240000 bytes. This is bad.

nsCookieService::Read does it, which maybe isn't so bad since it looks like we're reading all cookies. But it still seems kinda bad if the user has significantly less than 3000 cookies.

EnsureReadComplete and PurgeCookies allocate an AutoTArray of the same size on the stack, which seems bad. Such large data structures shouldn't be on the stack.
My recommendation is to switch to regular nsTArray and use the constructor that accepts an initial size as an argument instead: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#2096
Mentor: josh
Whiteboard: [lang=c++][good first bug]
I'd like to give this a shot as my first bug for my assignment. Would you be able to guide me through this?
Absolutely!
Assignee: nobody → steven.low
Great! So far I've got Mozilla built, but I'm fairly new to the codebase would you be able to point me towards a starting point?
You'll want to look at netwerk/cookie/nsCookieService.cpp for the methods referenced in comment 0.
Hey Josh, Sorry about the delay, I've had to put this on the back burner, but I expect to have a patch for you sometime next week.

Regarding the bug, I've found the referenced methods, and wanted to clarify a few things:
1) in comment 0 Robert mentions "nsCookieService::Read does it, which maybe isn't so bad since it looks like we're reading all cookies." So I'd like to know if I should be changing to nsTArray.
2) for switching the AutoTArray to nsTArray is there a specific initial size argument that I should be using?

Thanks!
1) I think leaving nsCookieService::Read alone makes sense.
2) Using kMaxNumberOfCookies to start with makes sense to me.
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][necko-would-take]
I'm not 100% sure I've made a Mercurial-ready patch properly in git, so please let me know if I've done something wrong.
Attachment #8726054 - Flags: review?(josh)
Hi Josh, I'm not quite sure how to test this patch, can you recommend a procedure that I could try?
Comment on attachment 8726054 [details] [diff] [review]
Bug-1247548.patch

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

This patch looks right! Sorry for the delay; I was on vacation last week and I've been catching up. As for testing, you could run `./mach xpcshell-test extensions/cookie/test/unit/` for a quick smoketest. Do you have tryserver access yet? If not, you should apply for it: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server
Attachment #8726054 - Flags: review?(josh) → review+
Hi Josh, I figured you might have been on vacation, so no worries. The automated email I got said to have the person who reviewed the patch push it to the try server, would you instead like me to apply for access and push it myself?

Also as a lingering question, I'm not sure I entirely understand the difference between the nsTArray with initial size, and the AutoTArray.  Why does the nsTArray use less stack space?

Thanks!
AutoTArray<T, N> contains an inline array of N elements of type T, and transparently switches to a heap-allocated vector if more than N elements are appended at any point in time. nsTArray<T> is a regular heap-allocated vector, and by using the constructor that accepts a count we pre-allocate N elements.

And yes, I think it's useful for you to have the ability to use our testing infrastructure. I'll vouch for the request if you make it :)
Hey Josh, I've applied for try server access here: https://bugzilla.mozilla.org/show_bug.cgi?id=1257545 would you mind vouching for me? thanks!
Do I have to have cloned the source with hg in order to push to the try server? I generated the patch from the git mirror.
I tried './mach try -b do -p all -u xpcshell -t none' from my local clone, and got 'remote: Permission denied (publickey).' Have I missed something in the setup?
What if you create a `config` file in ~/.ssh/ which contains
```
Host hg.mozilla.org
  User <commit_user_email_address>
  IdentityFile /path/to/private_key_file
```
Hey Josh, sorry to bother you again, that seemed to work but now I get the following error:

bug_1247548 0dd3837] try: -b do -p all -u xpcshell -t none
WARNING Pushing a new root
Bundling 38901 changesetsfatal: stream ends early
fast-import: dumping crash report to .git/fast_import_crash_3401
error: failed to push some refs to 'hg::ssh://hg.mozilla.org/try'
error: git-remote-hg died of signal 9
ERROR git command ['git', 'push', 'hg::ssh://hg.mozilla.org/try', '+HEAD:refs/heads/branches/default/tip'] returned 1

I'm not yet familiar enough with try to understand what I'm doing wrong.
I recommend consulting with the folks on IRC in #git, since that's outside my area of experience, unfortunately.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef48e50010010bd30ea94ce07116d82b12afebd7
Bug 1247548 - Changed nsCookieService::EnsureReadComplete and nsCookieService::PurgeCookeis to allocate nsTArray instead of AutoTArray. r=jdm
I went ahead with merging your changes given the technical difficulties you've been having. Thanks!
https://hg.mozilla.org/mozilla-central/rev/ef48e5001001
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.