I suspect there are a lot of add-ons out there that rely on window IDs being distinct across processes. We probably assume this in our own code in a few places. So I think it would make sense to honor this assumption. One option would be to allocate 16 bits of the window ID for a process ID. Then we'd still have 48 bits left over, which is plenty. We'd still have to assign a process ID, but I imagine that wouldn't be too hard using a table or something. Alternatively, the content process could request 1000 window IDs at a time using a sync call. That would be simpler but would introduce a sync call.

Could we just use the system pid? Do we need to worry about pid reuse over long timescales? Sync calls are probably bad. Kan-Ru has been doing a bunch of work to remove them in the startup process (although I guess you could preallocate a block for each content process that would generally be big enough, especially on b2g).

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 1/22-2/8) from comment #1) > Could we just use the system pid? Do we need to worry about pid reuse over > long timescales? yes. 16bit for process ID doesn't sound too much. 24/40 perhaps? ContentParentId is effectively 64bits, but we don't probably need all that.

It just occurred to me that we really only have about 53 bits of precision because most of these window IDs will get converted to doubles for JS. That's probably enough though. We could use the low 24 bits for the process ID and the remaining 29 bits for the window ID. Even if you allocate 10 windows per second, you would have to go 621 days before you run out. I'm curious though. Where do we assume that window IDs are never reused? smaug's comment makes it sound like that's a problem.

It is supposed to be an ID of a window. If it can be and ID of a window Foo or window Bar, the API is rather broken. I wouldn't be surprised if some chrome code has ID->window mapping without explicitly clearing that when window goes away. You can use ID->window as a weak ref. But I don't know whether we have anything like that.

(In reply to Olli Pettay [:smaug] from comment #4) > It is supposed to be an ID of a window. If it can be and ID of a window Foo > or window Bar, the API > is rather broken. > I wouldn't be surprised if some chrome code has ID->window mapping without > explicitly clearing that That seems like it would be a serious memory leak. But I guess that doesn't mean it doesn't happen.

Why would it be a memory leak. You just have an ID. The later something happens and you get a reference to a window and then check whether that window's id is the same one you're interested in.

If there's a map from ID->window, and if the entry doesn't go away when a window dies, then I assume it lives forever. So that would be the leak. Do you mean that the entry would be removed when the window is garbage collected, but not when the window is closed? If so, then it seems like we could recycle the window ID after the window's destructor runs.

ID is just ID. I didn't mean ID->window is any strong reference. It is just that code may expect that if it has ID, it always points to window Foo, (and the code _may_ later get access to that window Foo).

It might not be huge but the array of mappings will keep growing and "leak" in that case.

Where you get the idea of having any array. A js component could just keep an ID for the whatever "current window", and next time something happens, say some DOM event, it just checks if the event is coming from the same window using that ID. If the ID has changed, it knows the thing it was doing isn't valid anymore and can start a new thing with the new window. So, sort-of a weak ref.

I ended up using 22 bits for the process ID. That's enough to create one process per second for 48 days. I feel like we should use more bits for the window ID because an attacker (who might be trying to force us to recycle a window ID) probably has more control over creating windows that over creating processes.

Comment on attachment 8555502 [details] [diff] [review] window-id >+ uint64_t processBits = processID & ((uint64_t(1) << kWindowIDProcessBits) - 1); >+ MOZ_RELEASE_ASSERT(processBits == processID); Rather odd check. I would have expected something like MOZ_RELEASE_ASSERT(processID < (uint64_t(1) << kWindowIDProcessBits)) >+ uint64_t windowBits = windowID & ((uint64_t(1) << kWindowIDWindowBits) - 1); >+ MOZ_RELEASE_ASSERT(windowBits == windowID); ditto But either way.

Backed out for Windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4039a0ba6c https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6036650&repo=mozilla-inbound