Closed Bug 1287626 Opened 4 years ago Closed 4 years ago

Add helper to generate a globally unique ID / process ID

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(e10s+, firefox50 affected, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
e10s + ---
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1287245 is caused by the incorrect assumption that an incrementing global JS variable is unique, even across processes.
To fix this, we need a way to obtain an ID that is guaranteed to be unique regardless of the process.

:billm suggests to publish the child ID @ http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/dom/ipc/ContentParent.cpp#2174 via nsIMessageManager.idl.
Whiteboard: triaged
Attachment #8779921 - Flags: review?(wmccloskey)
Attachment #8779922 - Flags: review?(wmccloskey)
Blocks: 1288279
Comment on attachment 8779921 [details]
Bug 1287626 - Add globally unique nsIXULAppinfo.uniqueProcessID

https://reviewboard.mozilla.org/r/70808/#review68534

::: xpcom/system/nsIXULRuntime.idl:90
(Diff revision 1)
>     * The system process ID of the caller's process.
>     */
>    readonly attribute unsigned long processID;
>  
>    /**
> +   * A unique ID of the caller's process, globally unique across processes.

Please mention that these IDs are not recycled, unlike the "processID" attribute above it.

::: xpcom/system/nsIXULRuntime.idl:92
(Diff revision 1)
>    readonly attribute unsigned long processID;
>  
>    /**
> +   * A unique ID of the caller's process, globally unique across processes.
> +   */
> +  readonly attribute unsigned long uniqueProcessID;

Please make this an unsigned long long. Then the C++ type will be uint64_t. We'll only get about 56 bits in JS, but that's fine.
Attachment #8779921 - Flags: review?(wmccloskey) → review+
Comment on attachment 8779922 [details]
Bug 1287626,1288279 - Make IDs unique across processes

https://reviewboard.mozilla.org/r/70810/#review68536

::: toolkit/components/extensions/ExtensionUtils.jsm:152
(Diff revision 2)
>  class BaseContext {
>    constructor(extensionId) {
>      this.onClose = new Set();
>      this.checkedLastError = false;
>      this._lastError = null;
> -    this.contextId = ++gContextId;
> +    this.contextId = `${++gContextId}${Services.appinfo.uniqueProcessID}`;

We should have a dash in between the two IDs to avoid confusion if, for example, ++gContextId is 1 and the process ID is 12 versus ++gContextId is 11 and the process ID is 2. Same for the other ones too.
Attachment #8779922 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6e0577a5a59a
Add globally unique nsIXULAppinfo.uniqueProcessID r=billm
https://hg.mozilla.org/integration/autoland/rev/a327a15f5d55
1288279 - Make IDs unique across processes r=billm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e0577a5a59a
https://hg.mozilla.org/mozilla-central/rev/a327a15f5d55
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.