Closed Bug 987923 Opened 6 years ago Closed 6 years ago

GetTempDir on OS X can be called without necessary autorelease pool in place

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 1 obsolete file)

1.42 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Attached patch Fix v1.0 (obsolete) — Splinter Review
From ipc/chromium/src/base/file_util_mac.mm:

bool GetTempDir(FilePath* path) {
  NSString* tmp = NSTemporaryDirectory();
  if (tmp == nil)
    return false;
  *path = FilePath([tmp fileSystemRepresentation]);
  return true;
}

The way this method is written, it depends on there being an autorelease pool in place. The assumption was probably made that this function will only be called with a event loop autorelease pool in place. There is no reason there has to be an event loop autorelease pool in place though - we're currently running into problems using this method where there is none. Best to put a local autorelease pool in place.
Assignee: nobody → joshmoz
Comment on attachment 8396631 [details] [diff] [review]
Fix v1.0

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

Requesting review from smichaud as he's familiar with autorelease pools.
Attachment #8396631 - Flags: review?(smichaud)
Attachment #8396631 - Flags: review?(smichaud) → review+
Ben Turner would like to have autorelease pools in place for all events on non-main threads. Patch coming up.
Attached patch FIx v2.0Splinter Review
Ben Turner wanted me to also add autorelease pools to the non-main-thread event loop.
Attachment #8396631 - Attachment is obsolete: true
Attachment #8396656 - Flags: review?(smichaud)
Comment on attachment 8396656 [details] [diff] [review]
FIx v2.0

There is some small chance draining the autorelease pool in the message pump will cause trouble.  But let's land this and see what happens.
Attachment #8396656 - Flags: review?(smichaud) → review+
(In reply to Steven Michaud from comment #4)

> There is some small chance draining the autorelease pool in the message pump
> will cause trouble.  But let's land this and see what happens.

That's why I didn't add it initially but all positive and negative consequences considered I'm in favor of it.
https://hg.mozilla.org/mozilla-central/rev/8a7b41d26e66
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.