Closed
Bug 628612
Opened 14 years ago
Closed 14 years ago
Facebook Unable to read messages
Categories
(Core :: JavaScript Engine, defect)
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
heycam
:
review+
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wesongathedeveloper
:
review+
|
Details | Diff | Splinter Review |
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
Keywords: regression,
top50
Comment 3•14 years ago
|
||
tracemonkey regression range: http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=5a6e9e7e487a&tochange=bbcc51fa912b
This fails with JM/TM disabled; likely bug 614137.
Comment 4•14 years ago
|
||
Web Console: "can't redefine non-configurable property 'Rect' @ http://www.facebook.com/?sk=messages:2"
Bug 577325 seems more likely then.
Comment 5•14 years ago
|
||
Comment 6•14 years ago
|
||
Confirming. I'm not able to use chat, and notifications are not working.
blocking2.0: ? → betaN+
Whiteboard: [hardblocker]
Comment 7•14 years ago
|
||
There's a "Rect" property on the Window object; it's the DOM Rect constructor....
Comment 8•14 years ago
|
||
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?
Comment 9•14 years ago
|
||
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#
Comment 10•14 years ago
|
||
(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?
Comment 11•14 years ago
|
||
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
Comment 12•14 years ago
|
||
> 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.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #506905 -
Flags: review?(jst)
Updated•14 years ago
|
Status: NEW → ASSIGNED
Comment 15•14 years ago
|
||
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+
Comment 16•14 years ago
|
||
Peter, I think you did most of the work on these constructors, any objections to making them configurable (removing JSPROP_PERMANENT)?
Comment 17•14 years ago
|
||
Re: comment 15: WebIDL follows de-facto standards here, not the other way around.
/be
Comment 18•14 years ago
|
||
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 19•14 years ago
|
||
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+
Comment 20•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee: general → dmandelin
Assignee | ||
Comment 21•14 years ago
|
||
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)
Assignee | ||
Comment 22•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/3489b017fd2a
http://hg.mozilla.org/mozilla-central/rev/75745dcb484f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker] → [hardblocker][fixed-in-tracemonkey]
Assignee | ||
Comment 23•14 years ago
|
||
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.
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #507013 -
Flags: review?(wesongathedeveloper)
Comment 25•14 years ago
|
||
Comment on attachment 507013 [details] [diff] [review]
Updates to mochitest test_canvas.html
Looks good!
Attachment #507013 -
Flags: review?(wesongathedeveloper) → review+
Comment 26•14 years ago
|
||
(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.
Reporter | ||
Comment 30•14 years ago
|
||
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
Comment 31•14 years ago
|
||
Myzar, can you please file a new bug on that?
Reporter | ||
Comment 32•14 years ago
|
||
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
Comment 33•14 years ago
|
||
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.
Description
•