859756 - [browserconsole] Missing nsIConsoleMessages from Browser Console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

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

STR:
1. Open the Browser Console after startup.
2. Open the Error Console
3. Compare

Expected:

Contents should be the same.

Actual:

Some items are missing, e.g., 

Could not read chrome manifest 'file:///Users/rob/Projects/FXTEAM/fx-team-working/obj-x86_64-apple-darwin12.3.0/dist/Nightly.app/Contents/MacOS/browser/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/chrome.manifest'.

OpenGL LayerManager Initialized Succesfully.
Version: 2.1 INTEL-8.10.44
Vendor: Intel Inc.
Renderer: Intel HD Graphics 4000 OpenGL Engine
FBO Texture Target: TEXTURE_2D

etc.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: working patch - no tests yet

This patch adds support for displaying nsIConsoleMessages in the Browser Console.

I also added a |timeStamp| property to all nsIConsoleMessages - otherwise they break sorting for cached messages in the console, and I find it's odd that they don't have any timestamp.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: part 1 - changes to nsIConsoleMessage

As briefly discussed on IRC this is how I monkey-patched* nsIConsoleMessage to add timeStamp, category and window ID properties. I also needed to do some changes to allow JS scripts to create new instances of nsIConsoleMessages, similar to how we can do it for nsIScriptErrors.

Please provide your feedback/review. Thanks!


* monkey-patching because I feel really small in the Gecko codebase, but I really enjoy the learning experience. Please let me know how I can improve the patch.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: part 2 - browser console changes

This patch changes the Web/Browser Console to accept nsIConsoleMessages just like nsIScriptErrors. We will display nsIConsoleMessages that belong to a specific window in the Web Console. In the Browser Console we show all such messages, irrespective of the window ID.

I was torn about reusing the "pageError" protocol packet or not. In the end I decided to add a "logMessage" packet. Thoughts / preferences?

I have added tests in toolkit/ and in browser/.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=54c951390b3b

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 749450 [details] [diff] [review]
part 1 - changes to nsIConsoleMessage

>+++ b/js/xpconnect/src/XPCModule.h

Why are the changes to this file needed?  I don't see anyone creating nsConsoleMessage by this new contract in this patch, and in any case adding component registration here for a component over in xpcom is not right.

>+++ b/js/xpconnect/src/nsScriptError.cpp
> nsScriptError::Init(const nsAString& message,
>+                       const char *category,
>+                       uint64_t innerWindowID)

Fix the indent, please.

>+  return InitWithWindowID(message, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""), 0, 0, 0, category, 0);

Why drop the innerWindowID?

Also, EmptyString() for the empty strings.

>+++ b/layout/build/Makefile.in
>+		   -I$(topsrcdir)/xpcom/base \

Why is this change needed?

>+++ b/xpcom/base/Makefile.in
>+		-I$(topsrcdir)/dom/base \

Likewise.

>+++ b/xpcom/base/nsConsoleMessage.cpp

It's too bad nsScriptError doesn't inherit from this class.  The code non-sharing makes me sad.

> nsConsoleMessage::nsConsoleMessage(const PRUnichar *message) 
>+  nsAutoString m;
>+  m.Assign(message);
>+  Init(m, nullptr, 0);

This is going to make two copies of the message, in many cases.  Can you at least use an nsString for 'm' to prevent that?

>+nsConsoleMessage::Init(const nsAString& message,
>+      if (outer)
>+	mOuterWindowID = outer->WindowID();

Please fix the indentation.  And I assume you have tabs in there.... you don't want that. Spaces only.

You presumably need to set things like mInnerWindowID and mOuterWindowID in the nsConsoleMessage constructor.  Also mIsFromPrivateWindow and mTimeStamp.

>+nsConsoleMessage::GetMessageMoz(PRUnichar **result)

This used to return mMessage. Why do something different now?  That looks wrong to me.

>+nsConsoleMessage::ToString(nsACString& /*UTF8*/ aResult)
>+    aResult.Assign(ToNewUTF8String(mMessage));

This leaks.  If you just want to return mMessage, then do:

  CopyUTF16toUTF8(mMessage, aResult);

>+++ b/xpcom/base/nsIConsoleMessage.idl
>+    void Init(in AString message, [optional] in string category, [optional] in unsigned long long innerWindowID);

Why uppercase 'I'?  Seems like you want "init" instead.

Or is the problem that you have an "init" on nsIScriptError already?  In that case, I would prefer "initMessage" here or something.

Comment 5

[Mihai Sucan [:msucan]]

Thanks for the quick review!

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 749450 [details] [diff] [review]
> part 1 - changes to nsIConsoleMessage
> 
> >+++ b/js/xpconnect/src/XPCModule.h
> 
> Why are the changes to this file needed?  I don't see anyone creating
> nsConsoleMessage by this new contract in this patch, and in any case adding
> component registration here for a component over in xpcom is not right.

nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? I guess we can add optional arguments to nsIConsoleService.logStringMessage() that take the category and the window ID.


> >+++ b/js/xpconnect/src/nsScriptError.cpp
> > nsScriptError::Init(const nsAString& message,
> >+                       const char *category,
> >+                       uint64_t innerWindowID)
> 
> Fix the indent, please.
>
> >+  return InitWithWindowID(message, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""), 0, 0, 0, category, 0);
> 
> Why drop the innerWindowID?
> 
> Also, EmptyString() for the empty strings.

Fixed.

> >+++ b/layout/build/Makefile.in
> >+		   -I$(topsrcdir)/xpcom/base \
> 
> Why is this change needed?
> 
> >+++ b/xpcom/base/Makefile.in
> >+		-I$(topsrcdir)/dom/base \
> 
> Likewise.

I was getting "nsConsoleMessage.h" include file not found errors throughout these folders. All these changes were needed once I added nsIConsoleMessage contractid in XPCModule.h. I didn't find a better place. Can you suggest a better approach?


> >+++ b/xpcom/base/nsConsoleMessage.cpp
> 
> It's too bad nsScriptError doesn't inherit from this class.  The code
> non-sharing makes me sad.

Good point. I tried to change nsScriptError to inherit from nsConsoleMessage but then I get errors about ambiguous inheritance. I'm missing something here.

> > nsConsoleMessage::nsConsoleMessage(const PRUnichar *message) 
> >+  nsAutoString m;
> >+  m.Assign(message);
> >+  Init(m, nullptr, 0);
> 
> This is going to make two copies of the message, in many cases.  Can you at
> least use an nsString for 'm' to prevent that?

Fixed.

> >+nsConsoleMessage::Init(const nsAString& message,
> >+      if (outer)
> >+	mOuterWindowID = outer->WindowID();
> 
> Please fix the indentation.  And I assume you have tabs in there.... you
> don't want that. Spaces only.
> 
> You presumably need to set things like mInnerWindowID and mOuterWindowID in
> the nsConsoleMessage constructor.  Also mIsFromPrivateWindow and mTimeStamp.

Fixed.

> >+nsConsoleMessage::GetMessageMoz(PRUnichar **result)
> 
> This used to return mMessage. Why do something different now?  That looks
> wrong to me.

Fixed.

> >+nsConsoleMessage::ToString(nsACString& /*UTF8*/ aResult)
> >+    aResult.Assign(ToNewUTF8String(mMessage));
> 
> This leaks.  If you just want to return mMessage, then do:
> 
>   CopyUTF16toUTF8(mMessage, aResult);

Fixed.

> >+++ b/xpcom/base/nsIConsoleMessage.idl
> >+    void Init(in AString message, [optional] in string category, [optional] in unsigned long long innerWindowID);
> 
> Why uppercase 'I'?  Seems like you want "init" instead.

Fixed.

Will update the patch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? 

Ah, that should be fine.  But in that case, please add to the right module!

> I didn't find a better place. Can you suggest a better approach?

nsConsoleMessage is in xpcom, so how about putting its registration in XPCOMModule.inc?

> I tried to change nsScriptError to inherit from nsConsoleMessage

Please don't.  It's sad that it does not, but not sad enough to warrant the pain involved in that change.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: part 1 - changes to nsIConsoleMessage v0.2

Updated the patch based on review comments. I didn't fix the includes / component registration, and the nsScriptError still doesn't inherit from nsConsoleMessage.

Can you please suggest the changes I need to make / point me to docs I should read about this stuff? Or some code I can look at. Also, I don't want my lack of experience with this code to take too much of your time by helping me. If needed, we can ping someone more experienced than myself with platform code to help us with this work.

Thank you!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: part 2 - browser console changes v0.2

Rebased the patch.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? 
> 
> Ah, that should be fine.  But in that case, please add to the right module!
> 
> > I didn't find a better place. Can you suggest a better approach?
> 
> nsConsoleMessage is in xpcom, so how about putting its registration in
> XPCOMModule.inc?

I didn't find said file. Thanks for pointing it to me. Will look into it. Thanks!

> > I tried to change nsScriptError to inherit from nsConsoleMessage
> 
> Please don't.  It's sad that it does not, but not sad enough to warrant the
> pain involved in that change.

Ah, ok. I saw it leads me down a path of pain... I thought it's caused by my noobism.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: part 1 - changes to nsIConsoleMessage v0.3

Fixed the component registration - this makes more sense. Thanks for pointing me to the right file (in my mxr searches I didn't find it).

Comment 11

[Mihai Sucan [:msucan]]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=53e0a55a9717

(yesterday's try push was broken by infrastructure problems.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750059 [details] [diff] [review]
part 1 - changes to nsIConsoleMessage v0.3

Yep, looks good!

Comment 13

[Mihai Sucan [:msucan]]

Attachment: part 1 - changes to nsIConsoleMessage v0.4

Had build failures in my try push. clang on my system allowed nsIScriptError to override the Init() virtual function from nsIConsoleMessage, but not gcc on try servers. I have renamed the new nsIConsoleMessage method to InitMessage().

Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=d57096658451

Comment 14

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/7ac5779a426c
https://hg.mozilla.org/integration/fx-team/rev/073791f33ab7

Comment 15

[Mihai Sucan [:msucan]]

dev-doc-needed for the new properties in nsIConsoleMessage: timeStamp, category, innerWindowID, outerWindowID and isFromPrivateWindow. Also note the new initMessage() method and contract ID for nsIConsoleMessage: "@mozilla.org/consolemessage;1".

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ac5779a426c
https://hg.mozilla.org/mozilla-central/rev/073791f33ab7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing intermittent Windows build failures.
https://hg.mozilla.org/mozilla-central/rev/ecdfb8bb501e

https://tbpl.mozilla.org/php/getParsedLog.php?id=23090070&tree=Mozilla-Inbound

Comment 18

[Boris Zbarsky [:bzbarsky]]

The problem is presumably that the nsGlobalWindow include is not OK in XPCOM, because XPCOM can get built before that header is exported?

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #18)
> The problem is presumably that the nsGlobalWindow include is not OK in
> XPCOM, because XPCOM can get built before that header is exported?

How can we fix this issue? If you believe the fix would require too many changes we can take out the window IDs and just keep timestamp and category.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The only reason you need nsGlobalWindiw is to call GetInnnerWindowWithId, right?

Can you use the nsIWindowMediator function instead?  Not sure if that would be exported early enough...  Benjamin?

Comment 21

[Mihai Sucan [:msucan]]

Ping? I would like to land this patch and the dependencies.

Comment 22

[Benjamin Smedberg]

It appears to me that the primary "problem" here is that nsGlobalWindow.h is not exported at all, so naturally you wouldn't be able to include it. Everyone who currently includes it is using LOCAL_INCLUDES to get at it, e.g. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Makefile.in#68

Other than it seems weird for any of this to be in XPCOM, it seems like whatever function you use ought to be in a public header, not a DOM-private one. Maybe we should move the entire consoleservice IDL+impl to somewhere in devtools?

Comment 23

[Mihai Sucan [:msucan]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #22)
> It appears to me that the primary "problem" here is that nsGlobalWindow.h is
> not exported at all, so naturally you wouldn't be able to include it.
> Everyone who currently includes it is using LOCAL_INCLUDES to get at it,
> e.g.
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Makefile.in#68

Can we fix this issue?

> Other than it seems weird for any of this to be in XPCOM, it seems like
> whatever function you use ought to be in a public header, not a DOM-private
> one. Maybe we should move the entire consoleservice IDL+impl to somewhere in
> devtools?

Should we do that here? My primary goal in this bug is to get nsIConsoleMessages displayed in the Browser Console. For that I needed the |timeStamp| property. While I was there I also added the |category| and window ID properties because this was something we were interested to do other times for the web console. If these changes require a lot of other work to happen we should probably split it into two bugs.

Thank you!

Comment 24

[Mihai Sucan [:msucan]]

Attachment: part 1 - changes to nsIConsoleMessage v0.5

As discussed on IRC I've updated the patch to only add the timeStamp property to nsIConsoleMessages, to avoid the problems with window IDs. I also removed |category| because it is not needed to get nsIConsoleMessages displayed in the browser console.

Thanks!

Comment 25

[Mihai Sucan [:msucan]]

Attachment: part 2 - browser console changes v0.3

Rebased the patch and updated tests to match changes in "part 1".

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9182fade429f

If it turns out to be green I will land these patches and bug 602006.

Comment 26

[Mihai Sucan [:msucan]]

Try push was green. Landed the two patches:

https://hg.mozilla.org/integration/fx-team/rev/a6c12acfa421
https://hg.mozilla.org/integration/fx-team/rev/ea16ea097418

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a6c12acfa421
https://hg.mozilla.org/mozilla-central/rev/ea16ea097418