612405 - Devise a better way to determine the web console is a 'native' console object

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[David Dahl :ddahl]

We want to notify the enduser if the console object has been paved over. We currently do that by checking if the console is instanceof HUDConsole. With the landing of the lazy console and integration patches ( bug 587734 ) we seem to not be able to check instanceof when the object is a nsIDOMGlobalPropertyInitializer. The current, cheezy approach is to do:

if (!(aContentWindow.wrappedJSObject.console.classID == CLASS_ID_VALUE)) {
       this.logWarningAboutReplacedAPI(hudId);
     } 

Of course the classID is not enumerable from content pages, a developer who would like to fool this check would have to figure it out from the HUDConsole.jsm source code. Still a pretty weak check though.

Comment 1

[Kevin Dangoor]

Is this bug to fix cheezyness or does this mean that the "console has been paved over" message won't get displayed after the lazy console lands? (the priority of the latter is higher than the former, to be sure!)

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> Is this bug to fix cheezyness or does this mean that the "console has been
> paved over" message won't get displayed after the lazy console lands? (the
> priority of the latter is higher than the former, to be sure!)

The message will get displayed - it is just a less than meaningful check if it truly is "our" console.

Comment 3

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

Right - as landed it could theoretically be replaced by the page in a way that we wouldn't detect, but the page would need to go through efforts to "hide" the replacement from us, so it isn't a huge issue in practice.

Comment 4

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

Attachment: WIP

This is what I had in mind, but it doesn't work. Will need to find a better way.

Comment 5

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

so the .classID check didn't work for this? Disappointing.

Any idea why the instanceof check isn't working in your version, gavin?

Comment 6

[David Dahl :ddahl]

I tried:

if (!(consoleObject instanceof Ci.nsISupports)) { 

as well. we should ask jst why this does not work.

Comment 7

[Nochum Sossonko [:Natch]]

Why not make |console| a a getter/setter prop on the window and have the setter display the message then overwrite |console|?

Comment 8

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

We want to continue making use of the "JavaScript Global Property" functionality, rather than having to define setters on every window global like we used to, so that's not a feasible solution.

Fixing bug 613315 and using getGlobalForObject seems like it could work, though.

Comment 9

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

Attachment: patch

Actually, it works fine now even without that. I also changed the test so that it actually tests the warning (rather than calling logWarningAboutReplacedAPI directly...), and also covers the other case (making sure the warning doesn't appear when it shouldn't). I also simplified testLogEntry() a bit.

Comment 10

[David Dahl :ddahl]

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

sweet. Cu.getGlobalForObject is pretty damn useful!

Comment 11

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 12

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

Attachment: patch

Comment 13

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

Requesting approval because this is well tested (in addition to simplifying a bunch of existing tests), and removes cruft from a newly-added web-exposed API (good to do now lest people somehow manage to start depending on it).

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/5928e59882b6