Closed
Bug 1072472
Opened 10 years ago
Closed 10 years ago
Sandbox shim broken by bug 1052093
Categories
(Firefox :: Extension Compatibility, defect)
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: billm, Assigned: billm)
Details
Attachments
(1 file, 1 obsolete file)
1.61 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
Bug 1052093 makes us go through security wrappers whenever the child wants to touch an object that lives in the parent. That broke the case where the parent asks the child to create a new Cu.Sandbox: the |options| parameter is passed as a CPOW pointing to the parent, and the child is no longer able to access is because of security restrictions. The fix is easy: just copy all the options properties to an object in the child. I added a test for this in bug 1072467.
Attachment #8494689 -
Flags: review?(mconley)
Comment 1•10 years ago
|
||
Comment on attachment 8494689 [details] [diff] [review] fix-greasemonkey-sandbox Review of attachment 8494689 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/addoncompat/RemoteAddonsParent.jsm @@ +580,5 @@ > .rootTreeItem > .QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIContentFrameMessageManager); > > + if (rest.length) { One of the options that one can send with Cu.Sandbox is "sandboxPrototype". According to the documentation: "A prototype object for the sandbox. The sandbox will inherit the contents of this object if it's provided." The rest of the options are strings or bools, so they transfer fine, but aren't we going to have a problem with sandboxPrototype? Will it not also be a CPOW from the parent scope that the child will have trouble accessing?
Comment 2•10 years ago
|
||
Comment on attachment 8494689 [details] [diff] [review] fix-greasemonkey-sandbox Review of attachment 8494689 [details] [diff] [review]: ----------------------------------------------------------------- Also note that I do see you testing sandboxPrototype in your test in bug 1072467, so I'm willing to believe that it's not a problem, but I'd like to understand why. ::: toolkit/components/addoncompat/RemoteAddonsParent.jsm @@ +583,5 @@ > > + if (rest.length) { > + let options = rest[0]; > + let optionsCopy = new chromeGlobal.Object(); > + for (let prop in options) { Could Components.utils.cloneInto be used here instead of manually copying each property? Or am I misunderstanding how cloneInto can be used?
Assignee | ||
Comment 3•10 years ago
|
||
In the Greasemonkey case, at least, sandboxPrototype points to a window in the content process. Since we're creating the sandbox in the content process, we don't have to worry about CPOWs at all there. Cu.cloneInto does a deep clone using structured cloning. It would probably try to recurse into the sandboxPrototype and clone its individual properties, which we definitely don't want. I can add comments about this.
Assignee | ||
Comment 4•10 years ago
|
||
Added a comment.
Attachment #8494689 -
Attachment is obsolete: true
Attachment #8494689 -
Flags: review?(mconley)
Attachment #8496137 -
Flags: review?(mconley)
Comment 5•10 years ago
|
||
Comment on attachment 8496137 [details] [diff] [review] fix-greasemonkey-sandbox v2 Review of attachment 8496137 [details] [diff] [review]: ----------------------------------------------------------------- Ok, understood. Thanks for clearing that up. :)
Attachment #8496137 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a029e4d3181b
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a029e4d3181b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in
before you can comment on or make changes to this bug.
Description
•