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

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

4 errors are shown in Error Console about it, when I want to open DownThemAll! Manager window.


Regression window (mozilla-inbound-win32-pgo)
Good:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32-pgo/1363493183/

Bad:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32-pgo/1363504162/

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=23df95aba9cd&tochange=d926c77d4cd7


Triggered by:
Bug 658909 - Incorrect |this| binding when using call() with XRayWrappers r=mrbkap

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Error Console.png

Comment 2

[Gabor Krizsanits (INACTIVE)]

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?

Comment 3

[Bobby Holley (:bholley)]

(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.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't assert against a failure in CanCallNow. v1

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.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Expand the this-fixup hack to include Cu. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Tests. v1

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b0a7e6aee6d7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in Bobby's mega-push caused Windows debug bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a5bbf32adb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

It appears the issue was a needs-clobber. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2391af8fecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1653ada4e251
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0529398287

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b2391af8fecb
https://hg.mozilla.org/mozilla-central/rev/1653ada4e251
https://hg.mozilla.org/mozilla-central/rev/dc0529398287

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Verified with latest Nightly.
Thanks!