850865 - Stop hijacking console in addon documents

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

We will have to stop hijacking console in addon's documents like panel, widget, ...
as soon as bug 587757. That's because the global console will receive messages.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Hmm Alex I'm not sure what we should do here, are you saying that we can remove https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/content/sandbox.js#L216-L262 ?

Comment 2

[Alexandre Poirot [:ochameau]]

Yes, I don't think we want the SDK console in these documents.
Instead we could use the regular console object, exposed in all documents, that will be in the Browser console for panel and widgets.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: https://github.com/mozilla/addon-sdk/pull/1950

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8598149 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1950

I'd like to get Mossop's r? too since this is affecting a test he recently added.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8598149 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1950

Did you verified this patch? Do the messages get correctly printed to browser console as before?
Also I'm wondering why you have to modify various code about console.debug?
console.debug exists on the non-SDK console object.
Otherwise it looks good, but I'm quite far from SDK these days...

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8598149 [details] [review]
> https://github.com/mozilla/addon-sdk/pull/1950
> 
> Did you verified this patch? 

I thought the tests were pretty good, and I prefer to rely on those.

This one made me pretty confident the change is working:

https://github.com/mozilla/addon-sdk/pull/1950/files#diff-81725957768748f6825a3dc1294fa77aR381

There appear to be three tests called "test:ensure console.xxx works in cs"

And this one was already ignoring the debug case for some reason: https://github.com/mozilla/addon-sdk/blob/master/test/addons/e10s-content/lib/test-content-worker.js#L385-L408

> Do the messages get correctly printed to
> browser console as before?
> Also I'm wondering why you have to modify various code about console.debug?

It turns out I just need to set this pref `"devtools.webconsole.filter.debug": true` and the debug stuff is logged.

> console.debug exists on the non-SDK console object.

Yes it seems to be deprecated though https://developer.mozilla.org/en-US/docs/Web/API/console which is why I didn't think it mattered much at first, but I've updated the pull request now to include console.debug tests again.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8598149 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1950

I made some updates based on Alex's comments.  I think Alex's review will be enough now too.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8598149 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1950

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6)
> (In reply to Alexandre Poirot [:ochameau] from comment #5)
> > Comment on attachment 8598149 [details] [review]
> > https://github.com/mozilla/addon-sdk/pull/1950
> > 
> > Did you verified this patch? 
> 
> I thought the tests were pretty good, and I prefer to rely on those.

Yes, but you have to verify it still looks good.
The way console messages are displayed in the browser console can change
and you have to verify and acknowledge any change.

> > Also I'm wondering why you have to modify various code about console.debug?
> 
> It turns out I just need to set this pref
> `"devtools.webconsole.filter.debug": true` and the debug stuff is logged.
> 
> > console.debug exists on the non-SDK console object.
> 
> Yes it seems to be deprecated though
> https://developer.mozilla.org/en-US/docs/Web/API/console which is why I
> didn't think it mattered much at first, but I've updated the pull request
> now to include console.debug tests again.

Ah ok didn't knew about debug being deprecated... you can get rid of the related tests then, up to you.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8598149 [details] [review]
> https://github.com/mozilla/addon-sdk/pull/1950
> 
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6)
> > (In reply to Alexandre Poirot [:ochameau] from comment #5)
> > > Comment on attachment 8598149 [details] [review]
> > > https://github.com/mozilla/addon-sdk/pull/1950
> > > 
> > > Did you verified this patch? 
> > 
> > I thought the tests were pretty good, and I prefer to rely on those.
> 
> Yes, but you have to verify it still looks good.
> The way console messages are displayed in the browser console can change
> and you have to verify and acknowledge any change.

I wonder why there aren't tests for this if it matters? it would save human time, I'll do the manual regression test though.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

So here are my results:

- BC = Browser Console
- results for jpm and cfx were the same

# Old method:

1) console from add-on in BC and `cfx -v` stdout
2) console from content script in BC and `cfx -v` stdout
** “ERIK!!!!! 2”1 sandbox.js:327
3) console from add-on content in BC not in `cfx -v` stdout


# New method:

1) console from add-on in BC and `cfx -v` stdout
2) console from content script in BC and not in `cfx -v` stdout
** “ERIK!!!!! 2”1 javascript:console.log(‘ERIK!!!!! 2’):1:1
3) console from add-on content in BC and not in `cfx -v` stdout

So afaict there should have been a test that would have caught the regression in scenario #2, which basically results in the same issue that scenario #3 has as described in bug 982385.

This could make using cfx/jpm to debug more difficult.  The issue already exists for scenario #3 that would be created here for scenario #2 though so, I need to think about what is best here.

Perhaps we should continue to overload the console, and even overload it for all add-on content pages, which would resolve bug 982385.  Then we should have a test that all of these three scenarios can `console.log()` to stdout.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Any advice?

Is it possible that we won't have to overload a console and still be able to detect if a console.log came from our add-on? if not today then sometime down the road?

Comment 12

[Alexandre Poirot [:ochameau]]

I imagine we could handle messages coming from a sandbox nicely (like the one from content script) as sanboxes are flagged with metadata containing the addon id. And may be we could somehow flag the addon document's principal with similar metadata.
That would be great enhancement, but would require some tweaks in platform code.

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562