Closed
Bug 608651
Opened 13 years ago
Closed 13 years ago
Yelp javascript broken (maps/filtering) for 4+ months NS_ERROR_XPC_BAD_CONVERT_JS/NS_ERROR_XPC_BAD_OP_ON_WN_PROTO
Categories
(Tech Evangelism Graveyard :: English US, defect)
Tech Evangelism Graveyard
English US
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Mardak, Unassigned)
References
()
Details
(Keywords: regression, top500, Whiteboard: [Input])
On the latest builds in a new profile, the javascript errors prevent the map from loading and filtering on yelp search results: http://www.yelp.com/search Error: uncaught exception: [Exception... "Could not convert JavaScript argument" nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: http://media2.px.yelpcdn.com/static/201007092876478317/js/www-pkg-en_US.min.js :: anonymous :: line 158" data: no] There's also a bunch of yelp.event errors: Error: yelp.event is undefined Source File: http://www.yelp.com/search Line: 76 This started breaking yelp sometime in June (tracemonkey): 0601 ok 0701 bad wrapped 0801 bad wrapped 0901 bad convert 1001 bad convert At some point the exception switched to the current XPC_BAD_CONVERT from WrappedNative: Error: uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object" nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)" location: "JS frame :: http://media2.px.yelpcdn.com/static/201007092876478317/js/www-pkg-en_US.min.js :: anonymous :: line 158" data: no] http://input.mozilla.com/en-US/search/?product=firefox&q=yelp
Updated•13 years ago
|
blocking2.0: ? → beta8+
Reporter | ||
Comment 1•13 years ago
|
||
The line that this is breaking on is trying to access: window.HTMLElement.prototype.style e = "style" exception thrown at c[e] var e, d, b, a = window.SVGElement, c = window.HTMLElement; if (a && c) { a = a.prototype; c = c.prototype; for (e in c){ if (!(e in a) && Object.isFunction(d = c[e])) { if ((b = d + "").substr(b.indexOf("{")).replace(/\s/g, "") != "{[nativecode]}") { a[e] = d; } } } }
![]() |
||
Comment 2•13 years ago
|
||
Right, so the minimal testcase is: javascript:window.HTMLElement.prototype.style In 3.6, that returns Undefined. The switch to XPC_BAD_CONVERT happened when we went from using castNative to using castNativeFromWrapper; presumably as part of fast-unwrapping for nsIDOMCSSInlineStyle... But either way, why are we landing in this getter at all? In 3.6, we have: "style" in window.HTMLElement.prototype testing false, but on trunk it tests true? That's the real issue here, imo....
![]() |
||
Comment 3•13 years ago
|
||
Cameron, I have two questions: 1) What does WebIDL specify here in terms of the proto chain going forward? Should ElementCSSInlineStyle.prototype be on the proto chain of HTMLElement.prototype? 2) If so, then what should the "style" property getter do when invoked on HTMLElement.prototype?
Comment 4•13 years ago
|
||
(In reply to comment #3) > 1) What does WebIDL specify here in terms of the proto chain going forward? > Should ElementCSSInlineStyle.prototype be on the proto chain of DOM Level 2 Style isn't written with Web IDL, although CSSOM is -- CSSOM has this: http://dev.w3.org/csswg/cssom/#the-elementcssinlinestyle-interface with no indication of where the interface should be implemented. DOM 2 Style says: The expectation is that an instance of the ElementCSSInlineStyle interface can be obtained by using binding-specific casting methods on an instance of the Element interface when the element supports inline CSS style informations. So if we assumed CSSOM had either Element implements ElementCSSInlineStyle; or HTMLElement implements ElementCSSInlineStyle; then the style property would go on the mixin prototype object -- which would be an object that is the direct prototype of HTMLElement instances. If the interface had a [Supplemental] on it (even though [Supplemental] is not defined in the spec yet -- Ian and Anne have been using it) then it would mean that the prototype object for whatever is on the left of the "implements" would get the "style" property. That is as currently specified (and assuming [Supplemental] gets specified as it is being currently used). Usual caveats about various people not liking the way prototype chains are currently defined here. (Hopefully some progress on this can be made after meeting with TC39 folks in a couple of weeks.) > HTMLElement.prototype? > 2) If so, then what should the "style" property getter do when invoked on > HTMLElement.prototype? It should throw a TypeError, due to step 2 of the "attribute getter": http://dev.w3.org/2006/webapi/WebIDL/#dfn-attribute-getter since HTMLElement.prototype is just a regular native object, and not an instance of HTMLElement itself.
![]() |
||
Comment 5•13 years ago
|
||
Ok, so if the [Supplemental] thing happens, then our behavior is correct modulo the exception not being a TypeError.... (and yelp just needs to be fixed) Otherwise, it's wrong. Great. I guess for now we should keep the old behavior....
Comment 6•13 years ago
|
||
Seeing this on Input as well: http://input.mozilla.com/en-US/sites/theme/17690
Whiteboard: [Input]
Comment 7•13 years ago
|
||
Old behavior looks a de-facto standard we helped to set, no? Better stick with it for now, assuming we can ever change it. /be
![]() |
||
Comment 8•13 years ago
|
||
m-c range for ("style" in HTMLElement.prototype) becoming true: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=befec51e10ab&tochange=c7eaf9387b1f (matches comment 0 range). So this is a regression from bug 562008. The problem is that HTMLElement.prototype is now the direct proto of some elements, and we flatten all properties on the direct proto at the moment in XPConnect. So HTMLElement.prototype ends up with all the properties of NodeSelector, NSElement, EventTarget, ElementCSSInlineStyle, NSHTMLElement, etc on it. Peter, is there a sane way to stick another proto object between the sectioning elements and HTMLElement.prototype so they behave like all other HTML elements in that sense? We need to either do that, back out bug 562008, totally change how XPConnect proto chains work, or evangelize Yelp, imo.
Blocks: 562008
Updated•13 years ago
|
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
Comment 9•13 years ago
|
||
Is it only Yelp that counts on this? If we differ from past Firefox releases and all the other browsers, I smell trouble. Yet this bug is unassigned. Johnny, who can take this one? /be
OS: Mac OS X → All
Hardware: x86 → All
Comment 10•13 years ago
|
||
I suspect what we need to do here is give our "HTMLElement" element classes a different class in JS, which in turn inherits from HTMLElement. That way HTMLElement.prototype would match (more or less) what it was in 3.6, yet we could have HTMLElement elements as we do now. If need be we could fudge what toString returns on HTMLElements such that they return "[object HTMLElement]" even if the underlying class is something else (suggestions?). Mounir, I'm giving this to you, but I and/or peterv/bz can help here if needed.
Assignee: nobody → mounir.lamouri
Keywords: regression
Updated•13 years ago
|
Version: unspecified → Trunk
Comment 11•13 years ago
|
||
Note that HTML5 has a style attribute on the HTMLElement interface. If we follow that then 'style in HTMLElement.prototype' really should be true, no? If that's the case I think we should at least try to evangelize Yelp. Can we try to figure out if it's only Yelp? I discussed this with jst, not sure if we can spider for this. We'd need to be able to distinguish the direct property lookups on the prototype from just a general property lookup?
![]() |
||
Comment 12•13 years ago
|
||
> Note that HTML5 has a style attribute on the HTMLElement interface.
That's the [Supplemental] version from above. Is this final enough that we can depend on it not changing? If so, then yes we should evangelize Yelp...
Comment 13•13 years ago
|
||
After discussions with TC39 yesterday, Web IDL is likely going to change so that interfaces like ElementCSSInlineStyle will definitely have their properties go on to the interfaces they're defined to augment. So if we had Element implements ElementCSSInlineStyle; in the relevant spec (CSSOM I guess), then you would have `'style' in Element.prototype`. And if instead it was HTMLElement implements ElemnetCSSInlineStyle; then it would be `'style' in HTMLElement.prototype`. Which of these would be the case is up to the spec that defines ElementCSSInlineStyle.
![]() |
||
Comment 14•13 years ago
|
||
Given comment 11, that's actually irrelevant, unless HTML5 changes. So all that matters is whether properties end up on prototypes at all (which is still a big if; we'd need to convince at least the webkit folks). If they do, then Yelp's code is broken for "title" and the like, not just "style".
Comment 15•13 years ago
|
||
There seemed to be broad agreement here at the TC39 meeting about having `A implements B;` meaning that B's properties go on to A.prototype.
Comment 16•13 years ago
|
||
Sayrer, did you mark this bug has blocking b8 for a specific reason? It sounds like it would be better to wait for feedbacks on spec side before doing anything here.
![]() |
||
Comment 17•13 years ago
|
||
Cameron, it's not even a matter of what A implements B means. It's a question of what the |attribute| keyword means. HTMLElement definitely has properties on it in the IDL. If those end up as getter/setter pairs on HTMLElement.prototype (which is not what any shipping browser does right now for the specific case of HTMLElement, note, though we do put getter/setter pairs on other DOM prototypes) then this bug is evang. If |attribute| just leads to own properties on objects, then what the site is doing is fine... So we need to be pretty sure that making |attribute| into getter/setter pairs will stick, before evangelizing. Imo.
Comment 18•13 years ago
|
||
OK. Oliver said he was happy with attributes corresponding to accessor properties on prototypes.
Comment 19•13 years ago
|
||
Sounds like we need to evangelize hard, including telling them WebKit's DOM will change to break them and track the WebIDL spec. /be
Comment 20•13 years ago
|
||
For now I'd be fine with either turning this completely into evang or changing these elements' classinfo back into HTMLUnknownElement (since according to bz that apparently wouldn't break the html5test). I'd rather do that than special-case these with a custom toString and .constructor.
Comment 21•13 years ago
|
||
Chrome bug for putting attributes on prototypes: http://code.google.com/p/chromium/issues/detail?id=43394
Comment 22•13 years ago
|
||
And WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=49739
Comment 23•13 years ago
|
||
Over to tech evangelism then.
Assignee: mounir.lamouri → english-us
Component: DOM → English US
Product: Core → Tech Evangelism
QA Contact: general → english-us
Version: Trunk → unspecified
Assignee: blizzard → nobody
Component: English US → General
Product: Tech Evangelism → Firefox
QA Contact: english-us → general
Comment 26•13 years ago
|
||
Clearing blocking flag.
Assignee: nobody → english-us
Component: General → English US
Product: Firefox → Tech Evangelism
QA Contact: general → english-us
Assignee: english-us → nobody
Component: English US → General
Product: Tech Evangelism → Firefox
QA Contact: english-us → general
Assignee: nobody → english-us
Component: General → English US
Product: Firefox → Tech Evangelism
QA Contact: general → english-us
Comment 27•13 years ago
|
||
Apparently I failed to clear the beta8 flag previously. NOW it's cleared
Comment 30•13 years ago
|
||
No updates from Yelp so far.
Comment 31•13 years ago
|
||
Yelp says that it might be a few days, but the fix is in the pipeline.
Reporter | ||
Comment 32•13 years ago
|
||
Works for me now!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
Product: Tech Evangelism → Tech Evangelism Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•