Closed Bug 851895 Opened 8 years ago Closed 8 years ago

DownThemAll! doesn't work properly after landing patches from bug #658909

Categories

(Core :: XPConnect, defect)

22 Branch
x86_64
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla22

People

(Reporter: Virtual, Assigned: bholley)

References

()

Details

(Keywords: nightly-community, regression)

Attachments

(4 files)

Line that throws:
module("chrome://dta-modules/content/glue.jsm", this);

where module is:
const module = Cu.import;

I would guess Cu.import.module(...) would simply work. Or if Cu were bound to module as the this arg. So conceptually this is sort of Ok... module should be defined as Cu.import.bind(Cu). Problem is that this is a common patter all over the place :( since xray wrappers were bound by default. What I don't get is why did this work? There should be no xrays around in this example, so why was the |this| pointer ignored in GWNOJO in this case?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bobbyholley+bmo)
(In reply to Gabor Krizsanits [:krizsa :gabor] (out until March 25)) from comment #2)
> Line that throws:
> module("chrome://dta-modules/content/glue.jsm", this);
> 
> where module is:
> const module = Cu.import;
> 
> I would guess Cu.import.module(...) would simply work. Or if Cu were bound
> to module as the this arg. So conceptually this is sort of Ok... module
> should be defined as Cu.import.bind(Cu). Problem is that this is a common
> patter all over the place :( since xray wrappers were bound by default. What
> I don't get is why did this work? There should be no xrays around in this
> example, so why was the |this| pointer ignored in GWNOJO in this case?

Thanks for looking at that, gabor.

So, this is actually a known issue (that has nothing to do with Xrays), I just didn't realize that it would be so common.

Basically, we have a compat hack to fix up |this| in XPConnect, but only if the function has been pulled off an XPC_WN_NoHelper instance. This is because most things that aren't XPC_WN_NoHelper implement nsIClassInfo, which means that they have an XPCWrappedNativeProto, which means that the funk behavior in comment 2 never would have worked.

However, Components.utils implements nsIXPCScriptable but _not_ nsIClassInfo. So it ends up with a different JSClass, but has the same issues as XPC_WN_NoHelper.

So I'm going to fix up part 8 of bug 658909 to detect Cu as well, which should fix this bug.
Flags: needinfo?(bobbyholley+bmo)
This can definitely happen now if the |this| object is wrong. It's possible
that we should be checking this earlier, but as the code stands this assertion
is incorrect.
Attachment #727300 - Flags: review?(mrbkap)
Attachment #727302 - Flags: review?(mrbkap)
Comment on attachment 727300 [details] [diff] [review]
Part 1 - Don't assert against a failure in CanCallNow. v1

This will make a bunch of existing bugs be WORKSFORME.
Attachment #727300 - Flags: review?(mrbkap) → review+
Attachment #727301 - Flags: review?(mrbkap) → review+
Attachment #727302 - Flags: review?(mrbkap) → review+
Backed out because something in Bobby's mega-push caused Windows debug bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a5bbf32adb
Verified with latest Nightly.
Thanks!
Status: RESOLVED → VERIFIED
Assignee: nobody → bobbyholley
Version: Trunk → 22 Branch
You need to log in before you can comment on or make changes to this bug.