612861 - Some content is missing on www.msnbc.com, gmail reply is broken

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gary [:streetwolf52]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101116 Firefox/4.0b8pre
Build Identifier: 20101116135406

Some content is missing on www.msnbc.com.  The main header is missing, the local weather, Stock information, and probably some others.

Reproducible: Always




Regression range:

http://hg.mozilla.org/mozilla-central/rev/898ef162e026 -  Bad
http://hg.mozilla.org/mozilla-central/rev/ef83567ee8d8 -  Good

This appears in my error log with the 'bad' build:

Error: Permission denied to access property 'firebug'
Source File: http://www.msnbc.msn.com/
Line: 922

Error: Permission denied to access property 'firebug'
Source File: http://www.msnbc.msn.com/id/23297925
Line: 4

Error: DBMgr is undefined
Source File: http://www.msnbc.msn.com/
Line: 1534

Error: DBMgr is undefined
Source File: http://www.msnbc.msn.com/id/23191536
Line: 1

Error: Permission denied to access property 'firebug'
Source File: http://www.msnbc.msn.com/id/23284290?LNW.js
Line: 1

Error: DBMgr is undefined
Source File: http://www.msnbc.msn.com/
Line: 1916

Error: DBMgr is undefined
Source File: http://www.msnbc.msn.com/id/35109355
Line: 2

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Error: Permission denied to access property 'firebug'

The relevant source line is:

 if (!("console" in window) || !("firebug" in console)) {

The checkin range is:

  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ef83567ee8d8&tochange=898ef162e026

which is when we started adding our own console object to windows.  Sounds like it's being created in such a way that random property gets throw, which is broken.  That needs to be fixed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing the code author too.

Comment 3

[Rob Campbell [:rc] (:robcee)]

and adding jst who consulted and provided code.

Comment 4

[Rob Campbell [:rc] (:robcee)]

why would we be getting a permission denied error accessing that property? I see that too in today's build, just running | "firebug" in console | in the console's input line.

Comment 5

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #1)
> > Error: Permission denied to access property 'firebug'
> 
> The relevant source line is:
> 
>  if (!("console" in window) || !("firebug" in console)) {
>

11:11:18.633: Exception: Permission denied to access property 'firebug' Source File: http://www.msnbc.msn.com/ Line: 922, Column: 0 Category: content javascript
11:11:18.662: Exception: Permission denied to access property 'firebug' Source File: http://www.msnbc.msn.com/id/23297925 Line: 4, Column: 0 Category: content javascript
11:11:18.665: Exception: DBMgr is undefined Source File: http://www.msnbc.msn.com/ Line: 1548, Column: 0 Category: content javascript

(from the console)

That ("firebug" in console) check is a fairly common pattern to try to identify if Firebug's on a given browser. Looks like they're using this check to then null out the console object to "hide" from Firebug.

There's good testcase fodder in here.

Comment 6

[Dão Gottwald [:dao]]

Somewhat related to this, is the current plan to always provide the console object even if the console isn't being used? Seems like pages could get into a slower "debug" or "developer mode" this way.

Comment 7

[Rob Campbell [:rc] (:robcee)]

not really the best place to discuss this, dao, but the "lazy console" will produce a console object when it's asked for.

The next stage is create a service to capture logging information to display in the hudservice.

see bug 612658 and follow up there with questions.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> not really the best place to discuss this, dao, but the "lazy console" will
> produce a console object when it's asked for.

I was asking because if the console object wasn't always there in the first place, this bug wouldn't affect all users...

> The next stage is create a service to capture logging information to display in
> the hudservice.
> 
> see bug 612658 and follow up there with questions.

My point is that developers right now don't expect the console object to exist for ordinary users, so they could kick off crazy debug tasks beyond just logging simple strings. Bug 612658 doesn't really seem relevant in this context (as in, it won't help).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Dão, there's always a console object present in Opera and all webkit-based browsers, for what it's worth.  Not sure about IE.  But we've had pages break because developers assumed a console object _is_ always present.....

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> Dão, there's always a console object present in Opera and all webkit-based
> browsers, for what it's worth.

Ok, I'll be quiet then.

Comment 11

[David Dahl :ddahl]

This appears to be sandbox related, i think. Overwriting "window.console" is not the same as overwriting "console"

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The __exposedProps__ thing is what causes the random property gets to fail, I think. We can probably just avoid using it.

Comment 13

[David Dahl :ddahl]

(In reply to comment #12)
> The __exposedProps__ thing is what causes the random property gets to fail, I
> think. We can probably just avoid using it.

I used __exposedProps__ so we would only be exposing the API. I would think it better if content cannot access our internal methods.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Attachment: fix

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 491241 [details] [diff] [review]
patch

Please hold, this is buggy.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Of course, this will break the "console replaced" warning. Sigh! I suppose I should just add |classID: self.classID| to the returned object for the moment, pending a fix for bug 612405.

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 491244 [details] [diff] [review]
patch

this looks good.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

With comment 18 addressed. Sorry for the patch churn...

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: [checked-in] patch

Comment 22

[Rob Campbell [:rc] (:robcee)]

I don't think there's any problem with "leaking" the classID, fwiw. Pretty low on the danger scale.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491254 [details] [diff] [review]
[checked-in] patch

r=me

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Replying to mail in gmail fails with the same error message.

Comment 25

[Rob Campbell [:rc] (:robcee)]

ran a full set of mochitests here and tested the patch manually on msnbc and gmaps. Looks good to go. Ready to land...

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 491254 [details] [diff] [review]
[checked-in] patch

http://hg.mozilla.org/mozilla-central/rev/f31dab55e41a

Comment 28

[Lars Gunther]

(In reply to comment #9)
> Dão, there's always a console object present in Opera and all webkit-based
> browsers, for what it's worth.  Not sure about IE.  But we've had pages break
> because developers assumed a console object _is_ always present.....

FWIW, I think that should be fixed and filed bug 606793 on that one.

Comment 31

[Ed Morley [:emorley]]

Gmail broken in different ways today:
https://bugzilla.mozilla.org/show_bug.cgi?id=613153

Related?

Comment 32

[Rob Campbell [:rc] (:robcee)]

investigating

Comment 33

[Rob Campbell [:rc] (:robcee)]

Ed, I think this is unrelated.

In your about:config, try disabling methodjit.

javascript.options.methodjit.chrome = false.

Comment 34

[Rob Campbell [:rc] (:robcee)]

see bug 613153.