Closed Bug 674597 Opened 9 years ago Closed 9 years ago

abort if attempting to create an xpcom proxy for wrapped JS


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)




(1 file, 1 obsolete file)

With bug 667535 and bug 674571 fixed (and maybe a few more), we shouldn't be creating xpcom proxies for wrapped JS.  Doing so violates a lot of preconditions. This bug is to create a (release mode) check and abort if this actually occurs in the wild (extensions).  bsmedberg is trying to remove xpcom proxies altogether, so this is really just a safety check in the interim.
Depends on: 674571
No longer depends on: 674572
Attached patch patch (obsolete) — Splinter Review
On second thought, abort() may be a bit severe.
Attachment #548917 - Flags: review?(benjamin)
Attachment #548917 - Flags: review?(benjamin) → review+
Attached patch weaker assertSplinter Review
Try server has revealed two more cases of proxied wrapped JS (one an nsIPrompt, I forget the other), but both were from main thread to main thread, so I just weakened the check.
Attachment #548917 - Attachment is obsolete: true
Attachment #549220 - Flags: review?(benjamin)
Attachment #549220 - Flags: review?(benjamin) → review+
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
If I understand this correctly, add-ons shouldn't be using GetProxyForObject at all, right?
They might, but xpcom/proxy's days are numbered; bsmedberg wants to remove it entirely.
Some add-ons are using it right now. The question is whether we should flag this in the compatibility bump code, and under which circumstances. It would also be good to know how that code can be changed so that it doesn't use proxies.
The abort will hit if the GetProxyForObject is called to proxy wrapped JS (that is, an IDL interface implemented in JS) to some other thread.  This has always been unsafe since it runs the wrapped JS's scripted QI immediately (instead of on the destination thread); this bug just catches it eagerly.
Depends on: 678547
Blocks: 678987
Backed out to fix bug 678440 and a few other intermittent crashes:

See bug 674571 comment 11 for details.
Resolution: FIXED → ---
Blocks: 678637
Shouldn't this also get backed out of aurora (once landed on central)? As these events were either side of the merge?
(In reply to Mark Banner (:standard8) from comment #10)
Already done.
AFAICT the backout in comment #9 above fixed bug 678637 too. See also bug 678637 comment #17 for one easy test ("can I start ChatZilla over SSL?") to run the next time a patch lands on this bug.
Thanks for pointing that out Tony.  The fix that will enable the patches to reland (bug 675221) should nuke all possibility of this problem, but we'll make sure to retest.
Closed: 9 years ago9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 675221
No longer blocks: 684711
You need to log in before you can comment on or make changes to this bug.