862024 - Warning about replaced window.console API shows when content scripts do not change the object

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mihai Sucan [:msucan]]

STR:

1. load http://people.mozilla.org/~tglek/dashboard/
2. open the web console.
3. see warning:

> The Web Console logging API (console.log, console.info, console.warn, console.error)
> has been disabled by a script on this page.

This warning shows for no obvious reason.

Comment 1

[Mihai Sucan [:msucan]]

Problem identified - this is related to the latest Target.jsm changed, navPayload and such.

Patch here, 3 lines fix:
https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/bug-862024

Tomorrow I'll add a test and submit for review.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed fix

This is a regression caused by the recent Target API changes. This patch includes a fix and a test.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=463328323d7e

Thanks!

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Can you explain why the console can't rely on the target events for navigation, but has to use the protocol directly? This works for the debugger, so I must be missing something.

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #3)
> Can you explain why the console can't rely on the target events for
> navigation, but has to use the protocol directly? This works for the
> debugger, so I must be missing something.

This is something I wanted to ping you about yesterday. I wasn't sure which is the preferred way: target will-navigate/navigate events or the debugger protocol tabNavigated?

The problem at hand is that the web console needs the payload information, and that's not a recommended approach per Target.jsm comments (it's only there for non-remotable tools).

Why should we use the Target API's abstraction here? Do we plan to support other non-remotable targets?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #4)
> Why should we use the Target API's abstraction here? Do we plan to support
> other non-remotable targets?

The idea is to reduce code duplication and simplify tools, by moving common code in the target so that it gets reused by all of them. This way the style editor and the page inspector (and scratchpad if it's ever moved inside the toolbox) won't have to know about the navigation details of the remote protocol.

I think the problem is that I forgot to copy nativeConsoleAPI when constructing the event object in onRemoteTabNavigated. Does doing that fix this bug?

Comment 6

[Mihai Sucan [:msucan]]

Attachment: updated patch

Changed to use the Target API will-navigate and navigate events.

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #5)
> (In reply to Mihai Sucan [:msucan] from comment #4)
> > Why should we use the Target API's abstraction here? Do we plan to support
> > other non-remotable targets?
> 
> The idea is to reduce code duplication and simplify tools, by moving common
> code in the target so that it gets reused by all of them. This way the style
> editor and the page inspector (and scratchpad if it's ever moved inside the
> toolbox) won't have to know about the navigation details of the remote
> protocol.

As discussed on IRC I don't see any net benefit for this will-navigate/navigate abstraction on top of tabNavigated. I'd prefer will-navigate/navigate to be removed once all tools are remotable.

> I think the problem is that I forgot to copy nativeConsoleAPI when
> constructing the event object in onRemoteTabNavigated. Does doing that fix
> this bug?

Yes. I updated the patch accordingly. I don't feel strong about which way we do things.

Thanks!

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 738414 [details] [diff] [review]
updated patch

Review of attachment 738414 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, and sorry for the breakage!

Comment 9

[Mihai Sucan [:msucan]]

Thanks for the review!

Landed:
https://hg.mozilla.org/integration/fx-team/rev/4112b098c118

(last try push was green)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4112b098c118