Closed
Bug 631225
Opened 10 years ago
Closed 10 years ago
__exposedProps__ change breaks console.log.bind, et.al.
Categories
(Core :: XPConnect, defect, P1)
Tracking
()
RESOLVED
FIXED
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)
750 bytes,
patch
|
Details | Diff | Splinter Review | |
16.91 KB,
patch
|
Details | Diff | Splinter Review | |
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.
Reporter | ||
Comment 1•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
confirmed, using http://hg.mozilla.org/mozilla-central/rev/1c2d53a2dcfb.
Comment 3•10 years ago
|
||
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
Reporter | ||
Comment 4•10 years ago
|
||
Cc'ing Rob in case it's a JS issue.
Comment 5•10 years ago
|
||
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
Comment 6•10 years ago
|
||
applied to revert contents of bug 628410, part 1
Comment 7•10 years ago
|
||
applied to revert the changes from bug 628410, minus the test part.
Comment 8•10 years ago
|
||
confirmed. Backing out with the above two patches fixes the problem.
Updated•10 years ago
|
Assignee: nobody → jonas
Component: XPCOM → XPConnect
QA Contact: xpcom → xpconnect
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Why dont we add exposedProps where needed?
Comment 11•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
> 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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
> 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•10 years ago
|
||
(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•10 years ago
|
||
WIP patch, currently segfaults.
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #509954 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #509960 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #509968 -
Flags: review?(dtownsend)
Assignee | ||
Updated•10 years ago
|
Attachment #509968 -
Flags: review?(mrbkap)
Assignee | ||
Updated•10 years ago
|
blocking2.0: final+ → betaN+
Updated•10 years ago
|
Whiteboard: [hardblocker] → [hardblocker][has patch]
Comment 27•10 years ago
|
||
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+
Comment 28•10 years ago
|
||
(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".
Assignee | ||
Comment 29•10 years ago
|
||
Yeah, bind is not exposed. Thats the idea. We could try Object.bind.call though. Patrick, would that work?
Comment 30•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: jonas → gal
Assignee | ||
Comment 31•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #509968 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 32•10 years ago
|
||
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
Assignee | ||
Comment 33•10 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/6f74c581ca1e
Comment 34•10 years ago
|
||
(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.
Assignee | ||
Comment 35•10 years ago
|
||
Wow I meant to remove that. Weirdly enough though, I ran the console tests and it passed locally. Fixing.
Assignee | ||
Comment 36•10 years ago
|
||
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? ;-)
Assignee | ||
Comment 38•10 years ago
|
||
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•10 years ago
|
||
What about __noSuchMethod__ on the console object? Will that be affected?
Assignee | ||
Comment 40•10 years ago
|
||
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.
Assignee | ||
Comment 41•10 years ago
|
||
passed try, landed again http://hg.mozilla.org/tracemonkey/rev/9b510ceacb9d
Comment 42•10 years ago
|
||
Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/4c0ca7d630b9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•