Closed Bug 631225 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XPConnect, defect, P1)

Product:
Core
Component:
XPConnect
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dietrich, Assigned: gal)

References

Details

(Whiteboard: [hardblocker][has patch], fixed-in-tracemonkey)

Attachments

(3 files, 2 obsolete files)

part 1
750 bytes, patch
Details | Diff | Splinter Review
part 2
16.91 KB, patch
Details | Diff | Splinter Review
patch
6.45 KB, patch
mossop
: review+
mrbkap
: review+
Details | Diff | Splinter Review 
url: https://builder.addons.mozilla.org/addon/1000066/latest/

works in the previous nightly. broken today.
preliminary blocking, for investigation, in case it's a web compat issue.

post investigation, we can downgrade blocker status if necessary.
blocking2.0: --- → final+
Whiteboard: [hardblocker]
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
Blocks: devtools4
Priority: -- → P1
Cc'ing Rob in case it's a JS issue.
this appears to be a regression caused by the merging of bug 628410.

cc'ing jonas and andreas and blake and ...
Component: Developer Tools → XPCOM
Product: Firefox → Core
QA Contact: developer.tools → xpcom
Attached patch part 1 — — Splinter Review 
applied to revert contents of bug 628410, part 1
Attached patch part 2 — — Splinter Review 
applied to revert the changes from bug 628410, minus the test part.
confirmed. Backing out with the above two patches fixes the problem.
Assignee: nobody → jonas
Component: XPCOM → XPConnect
QA Contact: xpcom → xpconnect
re-summarized: was 20110202 nightly build breaks http://builder.addons.mozilla.com
Summary: 20110202 nightly build breaks http://builder.addons.mozilla.com → __exposedProps__ change breaks console.log.bind, et.al.
Why dont we add exposedProps where needed?
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.
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.
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.  :(
(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
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.
Blocks: 628410
(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.
> 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?
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.
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.
(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).
> 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.
(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.
Attached patch WIP patch. (obsolete) — — Splinter Review 
WIP patch, currently segfaults.
Attached patch patch (obsolete) — — Splinter Review
Attachment #509954 - Attachment is obsolete: true
Attached patch patch — — Splinter Review
Attachment #509960 - Attachment is obsolete: true
Attachment #509968 - Flags: review?(dtownsend)
Attachment #509968 - Flags: review?(mrbkap)
blocking2.0: final+ → betaN+
Blocks: 631725
Whiteboard: [hardblocker] → [hardblocker][has patch]
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),
  ...
}
Attachment #509968 - Flags: review?(dtownsend) → review+
(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".
Yeah, bind is not exposed. Thats the idea. We could try Object.bind.call though. Patrick, would that work?
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
Assignee: jonas → gal
Pushed with the improvement suggested by Dave (using bind).

http://hg.mozilla.org/tracemonkey/rev/e706761cecfd
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch], fixed-in-tracemonkey
Attachment #509968 - Flags: review?(mrbkap) → review+
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
(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.
Wow I meant to remove that. Weirdly enough though, I ran the console tests and it passed locally. Fixing.
Backed out. I will take this up with the try server instead of galling the tree.

http://hg.mozilla.org/tracemonkey/rev/6f74c581ca1e
"Galling"? Is that the result of applying too much Andreas Gal to the tree? ;-)
Galling the tree is defined as the repeated landing of insufficiently tested patches that turn the tree orange. Shaver defined the term last week ;)
What about __noSuchMethod__ on the console object? Will that be affected?
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.
passed try, landed again

http://hg.mozilla.org/tracemonkey/rev/9b510ceacb9d
Landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/4c0ca7d630b9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.