631225 - __exposedProps__ change breaks console.log.bind, et.al.

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

url: https://builder.addons.mozilla.org/addon/1000066/latest/

works in the previous nightly. broken today.

Comment 1

[Dietrich Ayala (:dietrich)]

preliminary blocking, for investigation, in case it's a web compat issue.

post investigation, we can downgrade blocker status if necessary.

Comment 2

[Rob Campbell [:rc] (:robcee)]

confirmed, using http://hg.mozilla.org/mozilla-central/rev/1c2d53a2dcfb.

Comment 3

[Kevin Dangoor]

The error visible in the console is: [09:54:33.655] window.console[name].bind is not a function @ https://builder.addons.mozilla.org/media/bespin/BespinEmbedded.js:4002

This is likely not something in the console code itself, but we should investigate where the problem is

Comment 4

[Dietrich Ayala (:dietrich)]

Cc'ing Rob in case it's a JS issue.

Comment 5

[Rob Campbell [:rc] (:robcee)]

this appears to be a regression caused by the merging of bug 628410.

cc'ing jonas and andreas and blake and ...

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: part 1

applied to revert contents of bug 628410, part 1

Comment 7

[Rob Campbell [:rc] (:robcee)]

Attachment: part 2

applied to revert the changes from bug 628410, minus the test part.

Comment 8

[Rob Campbell [:rc] (:robcee)]

confirmed. Backing out with the above two patches fixes the problem.

Comment 9

[Rob Campbell [:rc] (:robcee)]

re-summarized: was 20110202 nightly build breaks http://builder.addons.mozilla.com

Comment 10

[Andreas Gal :gal]

Why dont we add exposedProps where needed?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Jonas: I'm being told by robcee that this breaks a bunch of stuff (mootools, firebug, add on sdk builder) and he's suggesting a respin of b11. Agree? Let's get this in on mozilla-central, regardless.

Comment 12

[Rob Campbell [:rc] (:robcee)]

As dietrich noted in comment 0, this bug affects flightdeck and the addon builder, for starters.

Kevin Dangoor discovered that it also breaks the script menu in Firebug.

I expect there are a number of places on the web that might be affected by this, and given some time, think I could find a few. This is a regression we probably don't want to ship in a beta.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Can we somehow make the console.log that shows up in the webpage be compiled with the _page_'s principals, but then call the "real" console.log (with the the same |this| and arguments)?  Then pages can manipulate their version as desired...

I would have hoped that function proxies worked that way already, but apparently they don't.  :(

Comment 15

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #10)
> Why dont we add exposedProps where needed?

adding console.log.bind to the exposed props might fix the flight deck case. I'm just wondering what else might be broken and what fallout exposing bind() could have. Presumably we'll need to do that to the other functions as well. Also, apply(), call(), ... ?

Do you think this is only likely to be a problem for the console api?a

Comment 16

[Boris Zbarsky [:bzbarsky]]

What's breaking mootools (which particular property or properties)?  That needs to be fixed by changes on our end, not on the mootools end, obviously.

Comment 17

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #16)
> What's breaking mootools (which particular property or properties)?  That needs
> to be fixed by changes on our end, not on the mootools end, obviously.

I was speculating wildly and used mootools as a potential example because they do funky things to system objects. I haven't found any actual examples yet. Afaik, it's still just limited to the console.* functions.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> because they do funky things to system objects

What kinds of system objects?  Only things that are raw JS objects injected from chrome (console, the xpinstall trigger stuff) were changed in any way here.

So this is primarily a problem insofar as it affects content that touches such objects that we inject (does that cover anything other than console?) and extensions that inject things of their own for content to play with, right?

Comment 19

[Rob Campbell [:rc] (:robcee)]

bz, that sounds right.

ftr, Kevin isn't sure that this change is what broke firebug. Probably unrelated.

So that leaves us with just the console API atm.

Comment 20

[Patrick Walton (:pcwalton)]

I took a look at this today. Basically AFAICT we have two, rather unpleasant, options:

(a) Expose "bind" specially via __exposedProps__.

(b) Make console.log() and friends belong to the page's compartment, so they don't get wrapped. They can then talk to the console via events dispatched to the document, like Firebug does. Unfortunately, this will involve an "eval" or equivalent somewhere; the only way to do this that I know of is to access the page's eval (through arguments.caller.constructor perhaps) and create the appropriate function by evaling a string containing the function body. Structured clone will *not* work to copy functions across compartment boundaries, sadly.

Given the two options, I'm slightly inclined toward (a), since it's simple and it's not clear to me that (b) is significantly cleaner.

Comment 21

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #20)
> I took a look at this today. Basically AFAICT we have two, rather unpleasant,
> options:
> 
> (a) Expose "bind" specially via __exposedProps__.

This can cause security problems.

> (b) Make console.log() and friends belong to the page's compartment, so they
> don't get wrapped. They can then talk to the console via events dispatched to
> the document, like Firebug does. Unfortunately, this will involve an "eval" or
> equivalent somewhere; the only way to do this that I know of is to access the
> page's eval (through arguments.caller.constructor perhaps) and create the
> appropriate function by evaling a string containing the function body.
> Structured clone will *not* work to copy functions across compartment
> boundaries, sadly.

We talked about creating an event and listening for it in the HUDService. It should be possible, though I'm not sure why you think it requires an eval.

Jonas is cooking up another solution (yes, an invisible 3rd option!) that should get us there with minimal rearchitecting.

> Given the two options, I'm slightly inclined toward (a), since it's simple and
> it's not clear to me that (b) is significantly cleaner.

I will select option (c).

Comment 22

[Boris Zbarsky [:bzbarsky]]

> This can cause security problems.

Can it?  Seems like it should be fine with the new "nothing is exposed" default....  Not that I think we should do this, but I think it would work for the narrow bind() use case.

Comment 23

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #22)
> > This can cause security problems.
> 
> Can it?  Seems like it should be fine with the new "nothing is exposed"
> default....  Not that I think we should do this, but I think it would work for
> the narrow bind() use case.

I thought that was the whole reason we didn't want to do that. Morphing the calling context of console.log could do bad things.

In any case, there's no guarantee that would fix all the issues. We'd likely have to expose some of the other functions, (apply, call, etc) as well.

We'll see what Jonas comes up with today and proceed from there. If we have to implement the second option (I'll call it the events option) we can.

Comment 24

[Patrick Walton (:pcwalton)]

Attachment: WIP patch.

WIP patch, currently segfaults.

Comment 25

[Andreas Gal :gal]

Attachment: patch

Comment 26

[Andreas Gal :gal]

Attachment: patch

Comment 27

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 509968 [details] [diff] [review]
patch

Looks ok I guess. Would it work to just do:

return {
  enabled: x.enabled.bind(x),
  updateEnabled: x.updateEnabled.bind(x),
  ...
}

Comment 28

[Patrick Walton (:pcwalton)]

(In reply to comment #27)
> Comment on attachment 509968 [details] [diff] [review]
> patch
> 
> Looks ok I guess. Would it work to just do:
> 
> return {
>   enabled: x.enabled.bind(x),
>   updateEnabled: x.updateEnabled.bind(x),
>   ...
> }

Doesn't work on wrapped chrome objects (try it in the Web Console). I get "InstallTrigger.enabled.bind is not a function".

Comment 29

[Andreas Gal :gal]

Yeah, bind is not exposed. Thats the idea. We could try Object.bind.call though. Patrick, would that work?

Comment 30

[Patrick Walton (:pcwalton)]

Hmmm, seems to work in the Web Console:

[12:40:42.574] enabled = Function.bind.call(InstallTrigger.enabled, InstallTrigger)
[12:40:42.578] function () {[native code]}
[12:40:43.566] enabled()
[12:40:43.571] true

Comment 31

[Andreas Gal :gal]

Pushed with the improvement suggested by Dave (using bind).

http://hg.mozilla.org/tracemonkey/rev/e706761cecfd

Comment 32

[Andreas Gal :gal]

Backed out due to a test failure: http://hg.mozilla.org/tracemonkey/rev/b490110146ad

Re-landed with a fix for that (make __mozillaConsole__ not enumerable). 

http://hg.mozilla.org/tracemonkey/rev/dbafcb4a113e

Comment 33

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/6f74c581ca1e

Comment 34

[Jeff Walden [:Waldo]]

(In reply to comment #32)
> Re-landed with a fix for that (make __mozillaConsole__ not enumerable). 
> 
> http://hg.mozilla.org/tracemonkey/rev/dbafcb4a113e

Um.  That change doesn't do anything!  Defining over an existing property with a partial descriptor, conceptually, updates the existing property.  So:

  var o = { p: true };
  Object.defineProperty(o, "p", { value: false });

The property o.p was {[[Value]]:true,[[Enumerable]]:true,[[Configurable]]:true,[[Writable]]:true} before the define.  It will be exactly the same after, except [[Value]] will instead be false.  So I'm not sure how your change did anything.

Unrelatedly, the |return contentObject| that follows the evalInSandbox is over-indented by two spaces.

Comment 35

[Andreas Gal :gal]

Wow I meant to remove that. Weirdly enough though, I ran the console tests and it passed locally. Fixing.

Comment 36

[Andreas Gal :gal]

Backed out. I will take this up with the try server instead of galling the tree.

http://hg.mozilla.org/tracemonkey/rev/6f74c581ca1e

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

"Galling"? Is that the result of applying too much Andreas Gal to the tree? ;-)

Comment 38

[Andreas Gal :gal]

Galling the tree is defined as the repeated landing of insufficiently tested patches that turn the tree orange. Shaver defined the term last week ;)

Comment 39

[Nochum Sossonko [:Natch]]

What about __noSuchMethod__ on the console object? Will that be affected?

Comment 40

[Andreas Gal :gal]

The console object is a regular content object and can do tricks like __noSuchMethod__. The chrome object wrapped inside of it does not have __nuSuchMethod__ but doesn't need it either. Patrick has a patch to use proxies instead of __noSuchMethod__. You guys should use that.

Comment 41

[Andreas Gal :gal]

passed try, landed again

http://hg.mozilla.org/tracemonkey/rev/9b510ceacb9d

Comment 42

[Johnny Stenback (:jst)]

Landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/4c0ca7d630b9