Closed Bug 628612 Opened 14 years ago Closed 14 years ago

Facebook Unable to read messages

Categories

(Core :: JavaScript Engine, defect)

x86
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Myzar74, Assigned: dmandelin)

References

()

Details

(Keywords: regression, testcase, top50, Whiteboard: [hardblocker][fixed-in-tracemonkey][fx4-fixed-bugday])

Attachments

(3 files)

User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre After today TM-MC merge i'm unable to read messages on facebook Reproducible: Always Steps to Reproduce: 1. You must have a facebook account and have private messages archived or new 2. Click http://www.facebook.com/?sk=messages to show your messages 3. Actual Results: The messages pane is blank Expected Results: My messages would showup
Severity: normal → major
blocking2.0: --- → ?
Version: unspecified → Trunk
works 1295867012 broken 1295927751
Keywords: regression, top50
Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
tracemonkey regression range: http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=5a6e9e7e487a&tochange=bbcc51fa912b This fails with JM/TM disabled; likely bug 614137.
Web Console: "can't redefine non-configurable property 'Rect' @ http://www.facebook.com/?sk=messages:2" Bug 577325 seems more likely then.
Attached file Testcase (deleted) —
Keywords: testcase
Confirming. I'm not able to use chat, and notifications are not working.
blocking2.0: ? → betaN+
Whiteboard: [hardblocker]
Blocks: 577325
There's a "Rect" property on the Window object; it's the DOM Rect constructor....
Object.getOwnPropertyDescriptor(this, "Rect").toSource() => ({value:({}), writable:true, enumerable:false, configurable:false}) If the property were enumerable this would work. But it's not, so the redefinition fails because it would change property attributes of a non-configurable property. I'm not entirely sure what to do about this from a compatibility point of view. The old way doesn't work. But the new way also may not work. Allen, anyone else?
Don't know if this helps, but when you go to Facebook without an account and just look at a thread like the one below that I've been following, you are unable to advance the thread or read the "older posts". You are stuck on the first page. Facebook seems to run their threads differently from most where you click from page to page. Facebook entries keep loading as you scroll down the page. This works fine with Firefox 3.6.13, but not with the current nigthly of Minefield. It did work fine yesterday though. I'm on XP Pro SP3. http://www.facebook.com/uscensusbureau#
(In reply to comment #8) > If the property were enumerable this would work. But it's not, so the > redefinition fails because it would change property attributes of a > non-configurable property. > > I'm not entirely sure what to do about this from a compatibility point of view. > The old way doesn't work. But the new way also may not work. Allen, anyone > else? It should also work if the property was configurable. Ideally, 'Rect' would be defined as a property of Window.prototype. Then (according to the lastest fix to 10.5) the function declaration could still over-ride it via its [[DefineOwnProperty]] on the window object. An immediate fix, might be make "Rect" configurable:true. The function declaration would still work so facebook would be ok but the "Rect" becomes "deletable" which is apparently a change from legacy and contrary to the WebIDL draft. It seems like a hack but may be an ok immediate fix. Are there other window properties that are going to have the same issue?
I don't believe we have any internal C++-integrity requirement for these props to be non-configurable. They just are, due to JSPROP_PERMANENT use in days of yore. Johnny, do you recall more? Make 'em configurable if we can't move them to a prototype. /be
> Are there other window properties that are going to have the same issue? grep DOMCI_CLASS dom/base/nsDOMClassInfoClasses.h | wc -l 345 So yes. ;) Looks like webidl section 4.4 currently says that these are supposed to be defined as { [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false } properties on the ES global object. So putting them on Window.prototype would require a change to WebIDL. As would making them configurable. Or enumerable. I think making these configurable is the simplest fix for the issue, for what it's worth.
In Chrome (both latest release and canary), the test passes. There, |Rect| is: { configurable: true, enumerable: true, writable: false } If this works for them, presumably the simpler fix of making them configurable will work for us.
This fixes Facebook and the test case. Jeff, did you find out from your source that this is an OK thing to do?
Attachment #506905 - Flags: review?(jwalden+bmo)
Attachment #506905 - Flags: review?(jst)
Status: NEW → ASSIGNED
Comment on attachment 506905 [details] [diff] [review] Patch: make 'Rect' and things like it configurable The "source" was to be Cameron, since he curates the WebIDL spec, but I haven't seen him today. Maybe if necessary just land this without his a-okay, to fix it for people, then make sure he's okay with this precise fix after the fact? Hopefully nobody relies on these properties not being configurable, but stranger things have happened.
Attachment #506905 - Flags: review?(jwalden+bmo)
Attachment #506905 - Flags: review?(cam)
Attachment #506905 - Flags: review+
Peter, I think you did most of the work on these constructors, any objections to making them configurable (removing JSPROP_PERMANENT)?
Re: comment 15: WebIDL follows de-facto standards here, not the other way around. /be
http://twitter.com/#!/heycam/status/30029892577927168 suggests Cameron is having a day off, maybe. Agree with Waldo, we can get his + after the fact, even if he catches something de-facto or non-interoperable that he wants to do differently de-jure. We need to fix this bug pronto. /be
Comment on attachment 506905 [details] [diff] [review] Patch: make 'Rect' and things like it configurable (I'm off for Australia Day today, which I swapped with next Monday's Auckland Anniversary day.) Making Web IDL require the constructor properties be configurable would be consistent with other recent changes increasing the DOM's configurability in general. The only relevant properties that would be left non-configurable would be InterfaceName.prototype, Constructor.length, and constants. As David mentions, Chrome makes these properties configurable. WebKit and Opera do not. (Haven't got IE around to test atm.) I too would be surprised if making them configurable would break things. Of the three options -- moving to the prototype, making them enumerable and making them configurable -- my guess is that making them configurable is the safest, and it's probably easiest for other implementations to converge on, too.
Attachment #506905 - Flags: review?(cam) → review+
And it seems I already have a bug open to make this change: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11176
Assignee: general → dmandelin
Comment on attachment 506905 [details] [diff] [review] Patch: make 'Rect' and things like it configurable Thanks for the reviews. I'm glad we could get this fixed right away--I think it's been reviewed plenty at this point.
Attachment #506905 - Flags: review?(jst)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker] → [hardblocker][fixed-in-tracemonkey]
There is a mochitest, tests/content/canvas/test/test_canvas.html, that assumes these properties are non-configurable. I removed those subtests in a followup patch, because they are now "wrong". http://hg.mozilla.org/tracemonkey/rev/82727ded88e6 http://hg.mozilla.org/mozilla-central/rev/8b600831f115 I did it right away so TB wouldn't stay orange (and backing out would mean another day of FB not working in the nightlies). Someone should probably check on the test changes, though.
Attachment #507013 - Flags: review?(wesongathedeveloper)
Comment on attachment 507013 [details] [diff] [review] Updates to mochitest test_canvas.html Looks good!
Attachment #507013 - Flags: review?(wesongathedeveloper) → review+
(In reply to comment #13) > In Chrome (both latest release and canary), the test passes. There, |Rect| is: > > { configurable: true, enumerable: true, writable: false } Just to note since I neglected to do so earlier, as far as I know SpiderMonkey is the first engine to implement ES5 (plus errata) semantics for top-level function statements. Thus even if Chrome had the same attributes for Rect as we do/did, I doubt they would have been bitten by this bug. But I could well be wrong about other engines not having implemented ES5's function statement semantics -- if anyone can say otherwise, I'd love to know.
Blocks: 628937
looks like facebook is broken again with this hourly http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1296194306/ it works fine with yesterday nightly chat , messages and older posts don't work again , in the error console i've Error: Quickling._cache is undefined Source File: http://www.facebook.com/?ref=home Line: 4 and Error: Image corrupt or truncated: <unknown> Source File: <unknown> Line: 0 repeated many times
Myzar, can you please file a new bug on that?
I just tried with today nightly and i don't see the problem anymore, looks like a new checking or a backout fixed it. I'll file a new bug if i see it again
Testcase is PASS with Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre ID:20110204030345
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker][fixed-in-tracemonkey] → [hardblocker][fixed-in-tracemonkey][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: