583476 - WebConsole fails to activate when window.console is already defined

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

The WebConsole cannot be activated when the web page has already defined the window.console object.

This makes WebConsole fail on cnn.com and perhaps many other sites.

The problem: in HUDService.windowInitializer() the code checks if the window.console object is defined or not. If it's not, only then the WebConsole is displayed. I believe we should always overwrite window.console, and thus always enable the WebConsole without depending in any way on the page-defined window.console object.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix + test code

This is the proposed fix and test code. The patch should apply cleanly on mozilla-central.

This fix makes sure that the window.console object is always overwritten and that windowInitializer() does not fail when window.console is already defined.

Looking forward to your feedback!

Comment 2

[Julian Viereck]

(In reply to comment #0)
> I believe we should always overwrite window.console, and thus
> always enable the WebConsole without depending in any way on the page-defined
> window.console object.

We definitely should bring up the WebConsole no matter if there is already a console object on the page or not. However, I'm not sure if we should overwrite the existing window.console on the page with the one provided by the WebConsole. This could cause strange situations when a developer overwrote the console.log function to output stuff to an div and the WebConsole comes along. An interesting question is also, what should happen, when the WebConsole is closed again (should the overwitten console object be restored again?).

This bug is also related to the work done in bug 568629 (Small footprint, efficient global console service). When this bug is done, there will be an always-on console on the page.

Comment 3

[Mihai Sucan [:msucan]]

That is correct, Julian. I saw the relation between this bug and the bug you mentioned. Still, I am not sure this will affect the HUDService.windowInitializer() itself.

Presumably, in either way, the windowInitializer should no longer base the initialization on the existence of the window.console object.

Nonetheless, even when bug 568629 is done, the question of what to do with the window.console object still remains: should it be always overwritten by the Web browser code? Or not? If not, then there's some code in the HUDService that should be updated to *not rely* on the existence of the window.console object - since it can be a malicious object. (I am referring here to the setOnErrorHandler and perhaps other places I can't remember right now.)

Somehow, I believe that the web page author should be allowed to overwrite the window.console object (or modify it). Thus, I'd like to see the HUDService code having absolutely no reliance on the public window.console object.

Comment 4

[Kevin Dangoor]

I don't think the Web Console should just blow away the console object that is placed on the page by Firebug or the user. I do think you're right, Mihai, that the Web Console should function independently of the window.console object.

One other possibility to consider: if window.console is defined, the Web Console could proxy methods across so that a call to console.log, for example, can log to the Web Console *and* whatever the user had defined.

Comment 5

[Mihai Sucan [:msucan]]

Kevin: I agree that the WebConsole should receive messages from functions like console.log() - the independence I was talking about is only related to methods like that setOnErrorHandler which calls window.console.error. I believe that such methods should use internal HUDService functionality rather than go through the public API for displaying errors or other kinds of messages.

With regards to overwriting the window.console object from the HUDService: you have an interesting point. We could overwrite the window.console object and still call the original methods that the author provided. But what shall the code do when the page author does something like the one below?

window.console = "foo";

Now console.log() is not a function and the author did not intend for the console to be a console logger in our sense. Authors are not going to only overwrite the console.log/error/warn methods, they can make it whatever they want.

In such cases we could check if the window.console object contains log/warn/error/etc, and call the original methods when they exist. But, if we want to provide our console logging methods to the page when we encounter window.console = "foo", I think we are in a dilemma: do we overwrite the window.console property or not? We could be tricky and just let it as a string, and do something like window.console.log = ourCode() - which works, even if console = 'foo'.

Comment 6

[David Dahl :ddahl]

It is trivial for us to determine if we are dealing with our own console or Firebug. If we detect Firebug, we may want to display an error in our own console indicating that Firebug is already in use and to use it instead, otherwise close firebug to use Web Console.

That being said, we are constructing a new lower-level console that is basically a broadcaster for Web console's front end or Firebug to consume. see bug 568629

Comment 7

[David Dahl :ddahl]

So what I am saying is that once bug 568629 lands, we will not be attaching window.console to contentWindows any longer. The console will already exist and broadcast console.log/info/warn/error to any listening output UI like WebConsole or Firebug.

Comment 8

[Mihai Sucan [:msucan]]

Thus this bug depends on bug 568629? Should I mark it so?

Once that's fixed, my patch will be obsolete, is this correct? Should I expect that the problem will be solved with that patch? Or do I still need to do something in regards to the issue reported here? (WebConsole fails to activate when window.console is already defined)

Comment 9

[David Dahl :ddahl]

(In reply to comment #8)
> Thus this bug depends on bug 568629? Should I mark it so?
> 
not really.

> Once that's fixed, my patch will be obsolete, is this correct? Should I expect
> that the problem will be solved with that patch? Or do I still need to do
> something in regards to the issue reported here? (WebConsole fails to activate
> when window.console is already defined)

this is true.

Comment 10

[David Dahl :ddahl]

I spoke with jst about this issue. The main thing to keep in mind is that we DO NOT want to break websites. If the web developers of twitter and cnn have a console object they use for debugging, then let them have it. Breaking web sites is not good policy.

A better approach here is to check for a console that is not our console and leave it alone, while calling Cu.reportError("The Firefox Web Console cannot be opened because an existing console property already exists");

This may be a bit of a trick after the xpcom console component lands: bug 568629 - we will have to see. it may be easy! the issue will be timing.

I think this bug and 586803 might need to be combined.

Comment 11

[Julian Viereck]

(In reply to comment #10)
> A better approach here is to check for a console that is not our console and
> leave it alone, while calling Cu.reportError("The Firefox Web Console cannot be
> opened because an existing console property already exists");

Cu.reportError will show up in the ErrorConsole... which might get merged into the WebConsole? Which means the user will not see any hint? Even if we leave the error console there, a user will not understand why he can't open up the WebConsole because I expect that many developers won't open the ErrorConsole to look for an explanation.

Beside this, the WebConsole is getting much more useful even if it might not log the console.log/warn etc. functions. You can execute JS on the page, inspect JS objects and network traffic. When we open the WebConsole, can't we just open it and do a check if there is already a console object on the page? If there is, we don't attach our console object otherwise we do but that doesn't affect the other "bootstrapping" of the WebConsole.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to comment #11)
> (In reply to comment #10)
> > A better approach here is to check for a console that is not our console and
> > leave it alone, while calling Cu.reportError("The Firefox Web Console cannot be
> > opened because an existing console property already exists");
> 
> Cu.reportError will show up in the ErrorConsole... which might get merged into
> the WebConsole? Which means the user will not see any hint? Even if we leave
> the error console there, a user will not understand why he can't open up the
> WebConsole because I expect that many developers won't open the ErrorConsole to
> look for an explanation.
> 
> Beside this, the WebConsole is getting much more useful even if it might not
> log the console.log/warn etc. functions. You can execute JS on the page,
> inspect JS objects and network traffic. When we open the WebConsole, can't we
> just open it and do a check if there is already a console object on the page?
> If there is, we don't attach our console object otherwise we do but that
> doesn't affect the other "bootstrapping" of the WebConsole.

That's exactly my feeling as well.

The WebConsole is now soo much more than the window.console API. The WebConsole just needs to show the global messages somehow (Cu.reportError stuff, and other stuff that only show now in ErrorConsole). Anyhow, as I understand from Kevin, this issue is being tackled and there will be a solution.

The WebConsole should work without "caring" about the window.console API. We can still do lots of stuff in it, without the window.console API.

Comment 13

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

yeah, my feelings as well, to back up Mihai and Julian. Not showing the console because we weren't able to setup our window.console object seems... faily.

This probably requires rework to the way the Web Console starts up, but it might be nice if we could at least output the error message to that at startup and still give the user the ability to interact with the JS command line.

Comment 14

[Mihai Sucan [:msucan]]

I think there's not much rework needed, actually, for this bug fix. There's more rework needed for when bug 568629 is integrated. Once that's in, I think it will be a smooth ride to fix this one.

Comment 15

[Kevin Dangoor]

me too.

The actual reason I'm commenting is just to point out that the error suggested to be displayed on the Error Console should instead be displayed on the Web Console and that error sounds, to me, like a string that needs to be localized.

Comment 16

[David Dahl :ddahl]

(In reply to comment #13)
> yeah, my feelings as well, to back up Mihai and Julian. Not showing the console
> because we weren't able to setup our window.console object seems... faily.
> 

Right. I was suggesting we still show the console with that error at this point, just that we should not pave over the console that the web developer is using.

Comment 18

[Julian Viereck]

Attachment: Patch v1

I haven't seen that Mihai already supplied a patch therefor I wrote a new one from group up. My patch handles some more edge situations and refactors a little bit the code to make it less ugly.

I've also write a warning to the WebConsole if there is already a console defined on the page to let the user know that we don't override that one. The message looks currently like this:

  =!!= This page already has a console object defined, so logging from JavaScript to the Web Console will not take place =!!=

... which is likely to be changed if someone has a better idea (should the =!!= be removed?).

This patch depends on bug 587573, as in this bug a relation between contentWindow and a hud is introdcued that is useful for this patch. As bug 587573 should land within the b5 timeframe, I think this should be all right, otherwise I can break up the needed functionality and put it into this bug.

Comment 19

[David Dahl :ddahl]

Comment on attachment 467900 [details] [diff] [review]
Patch v1

We will have to again edit this code once bug 587734 lands, but looks good.

Comment 20

[Julian Viereck]

Attachment: Patch v1.1

Rebased patch that does some small code improvements as well.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 470455 [details] [diff] [review]
Patch v1.1

>+    for (var i = 0; i < childNodes.length; i++) {
s/var/let/

>+      let id = childNodes[i].getAttribute("id");
>+      if (id.split("_")[0] == "hud") {
please add a comment indicating what form the ids are in

>+    let hud;
>+    // If there is no HUD for this tab create a new one.
>+    if (!hudNode) {
>+      // get nBox object and call new HUD
>+      let config = { parentNode: nBox,
>+                     contentWindow: aContentWindow
nit: add trailing comma

>+      let hudWeakRef = Cu.getWeakReference(hud);
>+      HUDService.registerHUDWeakReference(hudWeakRef, hudId);
>+    }
>+    else {
>+      hud = this.hudWeakReferences[hudId].get();
I feel like this should be a helper method like "getHUDStrongReference" so you don't have to touch the member variable all over the place.  It's not immediately clear that you end up with a strong reference.  Please file a follow-up on this.

>+consoleAlreadyDefinedWarning==!!= This page already has a console object defined, so logging from JavaScript to the Web Console will not take place =!!=
You should check with UX about this string.  At the very least, this needs a localization note for the symbols "=!!=" at the start and end.  It's not clear that they don't have any meaning, and this is going to trip up localizers.  Might be good to ping Pike about this too.

r=sdwilsh

Comment 22

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #21)
> >+consoleAlreadyDefinedWarning==!!= This page already has a console object defined, so logging from JavaScript to the Web Console will not take place =!!=
> You should check with UX about this string.  At the very least, this needs a
> localization note for the symbols "=!!=" at the start and end.  It's not clear
> that they don't have any meaning, and this is going to trip up localizers. 
> Might be good to ping Pike about this too.

Yeah, I'd stay clear of trying to make ASCII graphics that look like programming language expressions. If we want to have different levels of warnings or equivalent, that should probably be done with coloring or separators that aren't in the actual text.

Comment 23

[Julian Viereck]

Attachment: Patch v1.2

Based on latest feedback from Shawn and Limi. Due to Limi's feedback I removed the "=!!=". We've got to decide if just adding a warning to the output is enough or if a stronger hint is necessary and fill a followup bug.

Comment 24

[Julian Viereck]

Need to investigate a problem a little bit more. Removing checkin-needed.

Comment 25

[Kevin Dangoor]

I think displaying a message (at the error level) should be enough, given that there won't be more messages appearing there.

Comment 26

[Julian Viereck]

Attachment: Patch B V1

This is small patch in addition to Patch V1.2.

Before, the console object used within the JSTerm object was taken from the window object when the sandbox was created. This is no longer working as the window might have a different console object. To fix this, the console object is passed to the JSTerm constructor directly.

Comment 27

[Shawn Wilsher :sdwilsh]

Comment on attachment 470792 [details] [diff] [review]
Patch B V1

>+  initializeJSTerm: function HS_initializeJSTerm(aContext, aParentNode, aConsole)
>   {
>     // create Initial JS Workspace:
>     var context = Cu.getWeakReference(aContext);
>     var firefoxMixin = new JSTermFirefoxMixin(context, aParentNode);
>-    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin);
>+    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin, aConsole);
>     // TODO: injection of additional functionality needs re-thinking/api
>     // see bug 559748
Er, why do we even do this?  These variables don't get held on to...

Comment 28

[David Dahl :ddahl]

(In reply to comment #27)
> Comment on attachment 470792 [details] [diff] [review]
> Patch B V1
> 
> >+  initializeJSTerm: function HS_initializeJSTerm(aContext, aParentNode, aConsole)
> >   {
> >     // create Initial JS Workspace:
> >     var context = Cu.getWeakReference(aContext);
> >     var firefoxMixin = new JSTermFirefoxMixin(context, aParentNode);
> >-    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin);
> >+    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin, aConsole);
> >     // TODO: injection of additional functionality needs re-thinking/api
> >     // see bug 559748
> Er, why do we even do this?  These variables don't get held on to...

The JSTerm constructor can be used to create a stand-alone "JS Workspace" as well as the console input.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> The JSTerm constructor can be used to create a stand-alone "JS Workspace" as
> well as the console input.
OK, I guess could someone explain or give pointers on how just creating it makes that work please?  It's a little bit of a strange concept (we may need to have a follow-up on it) as it is now.

Comment 30

[David Dahl :ddahl]

(In reply to comment #29)
> (In reply to comment #28)
> > The JSTerm constructor can be used to create a stand-alone "JS Workspace" as
> > well as the console input.
> OK, I guess could someone explain or give pointers on how just creating it
> makes that work please?  It's a little bit of a strange concept (we may need to
> have a follow-up on it) as it is now.

see: https://bugzilla.mozilla.org/show_bug.cgi?id=534398#c20

and http://img6.yfrog.com/img6/4179/screenshot008z.png

Comment 31

[David Dahl :ddahl]

(In reply to comment #29)
> (In reply to comment #28)
> > The JSTerm constructor can be used to create a stand-alone "JS Workspace" as
> > well as the console input.
> OK, I guess could someone explain or give pointers on how just creating it
> makes that work please?  It's a little bit of a strange concept (we may need to
> have a follow-up on it) as it is now.

also see: https://bugzilla.mozilla.org/show_bug.cgi?id=592361

Comment 32

[Shawn Wilsher :sdwilsh]

>+  initializeJSTerm: function HS_initializeJSTerm(aContext, aParentNode, aConsole)
>   {
>     // create Initial JS Workspace:
>     var context = Cu.getWeakReference(aContext);
>     var firefoxMixin = new JSTermFirefoxMixin(context, aParentNode);
>-    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin);
>+    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin, aConsole);
>     // TODO: injection of additional functionality needs re-thinking/api
>     // see bug 559748
Oh, I think I see what's going on now.  However, none of this code is actually doing anything now, right?  The work we are doing is just going to be thrown away, as far as I can tell, right?

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 470792 [details] [diff] [review]
Patch B V1

After talking with ddahl, I figured out what's going on here.

>   /**
>    * Initialize the JSTerm object to create a JS Workspace
This does more than initialize it, right?  It ends up attaching it to the right spot in the DOM via the mixin, right?  Can you please update this.

>+  initializeJSTerm: function HS_initializeJSTerm(aContext, aParentNode, aConsole)
>   {
>     // create Initial JS Workspace:
>     var context = Cu.getWeakReference(aContext);
>     var firefoxMixin = new JSTermFirefoxMixin(context, aParentNode);
>-    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin);
>+    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin, aConsole);
Also, a comment here indicating that th
It's confusing how these objects stay alive.  Please add a comment here.

>@@ -3439,26 +3441,28 @@ function JSPropertyProvider(aScope, aInp
>  *
>  *
>  *
>  * @param object aContext
>  *        Usually nsIDOMWindow, but doesn't have to be
>  * @param nsIDOMNode aParentNode
>  * @param object aMixin
>  *        Gecko-app (or Jetpack) specific utility object
>+ * @param object aConsole
>+ *        Console object to use within the JSTerm.
>  */
>+function JSTerm(aContext, aParentNode, aMixin, aConsole)
This documentation could also stand to be updated.

Have ddahl review the comment updates.  r=sdwilsh

Comment 34

[Julian Viereck]

*sigh* realized that this patch gone make things be more "false" then they are already in the WebConsole (e.g. if you apply the patch and reload the page the window bound to the JSTerm will not update. Before it updated to the new window BUT if there was an iFrame, it uses that iFrame window for execution). Will need to investigate this a little bit furture...

Comment 36

[David Dahl :ddahl]

(In reply to comment #34)
> *sigh* realized that this patch gone make things be more "false" then they are
> already in the WebConsole (e.g. if you apply the patch and reload the page the
> window bound to the JSTerm will not update. Before it updated to the new window
> BUT if there was an iFrame, it uses that iFrame window for execution). Will
> need to investigate this a little bit furture...

After looking at this patch, I think we need to make sure the console will always open and detect if  window.console has been replaced, notifying the user of the fact that console.log/info/warn/error have been usurped. 

we should also be collecting urls where this is a problem and what exceptions/errors are evident.

So far we have seen:

twitter.com homepage, when you are not logged in
cnn.com homepage
mint.com

Comment 37

[Patrick Walton (:pcwalton)]

(In reply to comment #34)
> *sigh* realized that this patch gone make things be more "false" then they are
> already in the WebConsole (e.g. if you apply the patch and reload the page the
> window bound to the JSTerm will not update. Before it updated to the new window
> BUT if there was an iFrame, it uses that iFrame window for execution). Will
> need to investigate this a little bit furture...

I'm not sure what this means; could you elaborate on this?

Comment 38

[David Dahl :ddahl]

(In reply to comment #37)

> I'm not sure what this means; could you elaborate on this?

I now think the root problem is that the HttpActivityObserver throws way before the UI is able to attach itself to the tab. 

The patch in this bug is not the correct way to solve this problem. The correct way to solve this problem is to not throw in the observer, and cache errors in the ConsoleStorageService. More on this later as I am still doing some investigation.

Comment 39

[David Dahl :ddahl]

So it turns out you can open the console on twitter and CNN, etc, it is a matter of timing as the netObserver is throwing which prevents the UI from showing. If you can hit the command early enough, you can get the console on twitter and cnn. Sometimes the UI is displayed but not resized properly probably because an exception stops the resizing command.  So we need to figure out how to show the UI regardless of exceptions in the network observer.

This actually ties in nicely with bug 567165 - we will need to create a holding tank for errors and console api events to wait for the UI to be displayed.

I have already created a proper storage service to hold them in bug 568629, which is reviewed and can land as soon as the next tracemonkey merge lands. (due to a crash bug 589329)

so to re-iterate:

1. make the network observer not throw errors or prevent the UI from showing.

  a. detect a non-firefox console and respectfully leave it alone, adding an INFO message to the console about this.

2. store real errors for unopened webConsole front end widgets, when they are displayed, fetch and display these messages waiting in the ConsoleStorageService.

3. In the meantime the only string I can think of that affects this bug is the INFO or WARN message to let the user know that:

 "we have detected a 3rd party window.console API, and any calls to console.info/warn/log/error will not be displayed in the webconsole"

Perhaps we need a separate bug for that: bug 592879

Comment 40

[David Dahl :ddahl]

Attachment: Testing Patch

Just a testing patch nothing to see here

Comment 41

[David Dahl :ddahl]

Attachment: test patch 2

Comment 42

[Kevin Dangoor]

This bug does not have strings any more... the string has been moved to bug 592879.

Comment 43

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 44

[Mihai Sucan [:msucan]]

I have looked into the new patches and comments of this bug. It is not very clear what I have to do here.


1. I initially provided a patch that went over the author-provided window.console object (always overwriting it). This came as a very simple and clear patch.

As I understand, this is not accepted - we must allows web page authors to have their own window.console objects - no problem.

2. Julian came with his patch which fixes the issue in a different way - one that does not overwrite the author-provided window.console object. What he did is take code from bug 580618 and use that to solve the issue in this report.

While I'd very much love to see bug 580618 fixed, I don't think this is the place to fix that. Also, there are some issues with jsterm that I am not sure they are fixed - as reported by Julian: the window object in jsterm is not updated. In some older patch for bug 578658 I included some more adjustments/fixes that solve jst erm issues.

Anyhow, bug 580618 is a much bigger fish to fry than this bug, and I believe that even this approach does not truly solve the issue of garbage collection of HUD objects. For context-awareness: what this patch does is it reuses existing HUD object weak refs, instead of always creating a new HUD object, for every content document global created (every new window object). This has nicely worked and it did solve the issue we had with GC, but this worked only before we fixed the HUDConsole to not expose the internal API. The HUDConsole object held a strong reference to the HUD object - at all times, and it was exposed to the public page. Since that fix (I cannot find the bug where the fix was included), we no longer do that, and the GC is able to clear the HUD object.

In the rebased/updated patches from bug 578658 I had to ultimately give up this approach to solving the GC issues of HUD objects. What I did is "blasphemous" in the latest patches - I gave up the use of weak refs altogether. I just store normal refs to the HUD objects. The reason why I did this is because, based on quite some testing, we still loose HUD objects if we use weak refs (for the aforementioned reason). The only way out of the issue was with real/strong refs. I believe that using strong refs, we can still avoid memleaks.

(As you can see the discussion is getting off-topic, but it's precisely because I believe we should not bite more than needed in the fix for the issue reported in this bug.)

3. David correctly points out an issue we have with the HTTP activity observer - it throws at times and that prevents the WebConsole UI to open. Still, I believe that's a different bug that deserves a different report - this is about the WebConsole failing to open when the window.console object is already defined.

4. David also points out we need a storage mechanism that is being worked on bug 568629, that will allow us to show errors that happen before the WebConsole is open, or while it opens. Shouldn't we open a separate report and make a patch for that? A patch that depends on bug 568629.

5. David reported bug 592879 which only shows a warning in the WebConsole output when the web page has already defined window.console. Still, users won't be able to see it because the WebConsole won't open at all. A fix to that bug doesn't make sense alone. We should fix that in this bug.

Someone please correct me if I am wrong. Maybe we should discuss this situation in a meeting.

What shall I do for this bug? Thank you, and sorry for this long, and perhaps boring, comment.

My thought is: I should fix this bug, as I intended initially, but with the minor change: we do no not overwrite the window.console object. This would avoid the hairy issue of GC stuff, from bug 580618, or the other http activity observer issue. (still there's a *slight* chance we may need to touch the GC problem *if* I cannot find an elegant way to solve the issue at hand)

Comment 45

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch.

I took Julian's patches (Patch v1.2 attachment 470758 [details] [diff] [review] and Patch B V1 attachment 470792 [details] [diff] [review]), merged them into one, and I included further fixes to issues related to JSTerm. All tests pass, no problems. It seems to work fine for me.

Note that this patch, as Julian intended, includes the work he did for bug 580618, with additional fixes. I am not sure if this patch fixes that issue as well (see previous comment for further details), but it does, certainly, fix the issue reported in this bug. I can now open the WebConsole on twitter.com and the other pages. Additionally, the changes that this patch does are useful - in the sense that we do not create instances of the HeadsUpDisplay object every time the window global object is created.

(For bug 580618 I'll submit a different patch, with a different approach.)

Comment 46

[Axel Hecht [:Pike]]

This is a [strings] blocker now and needs to go in by Friday.

Comment 47

[Shawn Wilsher :sdwilsh]

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

> >   /**
> >    * Initialize the JSTerm object to create a JS Workspace
> This does more than initialize it, right?  It ends up attaching it to the right
> spot in the DOM via the mixin, right?  Can you please update this.
> 
> >+  initializeJSTerm: function HS_initializeJSTerm(aContext, aParentNode, aConsole)
> >   {
> >     // create Initial JS Workspace:
> >     var context = Cu.getWeakReference(aContext);
> >     var firefoxMixin = new JSTermFirefoxMixin(context, aParentNode);
> >-    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin);
> >+    var jsTerm = new JSTerm(context, aParentNode, firefoxMixin, aConsole);
> It's confusing how these objects stay alive.  Please add a comment here.
These comments from comment 33 still apply to the current patch.  Please address them, and have ddahl review the comment updates.  r=sdwilsh

Comment 48

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Changes:

- it now depends on the string from bug 592879.
- changed some comments, as requested by Shawn. Thanks for your review, Shawn!

I believe this is ready for checkin, along with the patch from bug 592879. Still, requesting feedback+ from ddahl, as asked by Shawn.

Comment 49

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch as requested by ddahl (removed a comment).

Comment 50

[David Dahl :ddahl]

Needs-rebase:

anarchy ~/code/moz/mozilla-central/mozilla-central % hg qpush
applying console-opens
patching file toolkit/components/console/hudservice/HUDService.jsm
Hunk #4 FAILED at 2656
1 out of 11 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/HUDService.jsm.rej
patching file toolkit/components/console/hudservice/tests/browser/Makefile.in
Hunk #1 FAILED at 41
Hunk #2 FAILED at 58
2 out of 2 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/tests/browser/Makefile.in.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh console-opens

Comment 51

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

Comment 52

[David Dahl :ddahl]

This is indeed to risky to push:

INFO | runtests.py | Running tests: end.
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_581231_close_button.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_585237_line_limit.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_588967_input_expansion.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_consoleonpage.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_consoleonpage.js | Found a tab after previous test timed out: http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-own-console.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js | WebConsole was opened - Got 3, expected 1
make: *** [mochitest-browser-chrome] Error 1

Comment 53

[David Dahl :ddahl]

Attachment: rebased by ddahl

Comment 54

[Mihai Sucan [:msucan]]

Comment on attachment 474256 [details] [diff] [review]
rebased by ddahl

Thanks David for rebasing the patch. I'll come with fixes for possible memleaks - there were some reported by the tryserver yesterday. I'll fix them, push to try, and only then update this patch.

Comment 55

[Mihai Sucan [:msucan]]

David: with regards to the errors you pasted, how did you get them? which patches did you use? There were no such errors on my system, nor in the tryserver - only memleaks. I have addressed the memleaks now and I push to the tryserver for another round. *crossing fingers* :)

Comment 56

[Mihai Sucan [:msucan]]

Attachment: updated patch

Rebased patch. This patch is rebased on top of mozilla-central and on top of patch v0.3 from bug 592879 (attachment 473075 [details] [diff] [review]). Patch changes:

- fix the variable name in getConsoleOutputNode() for bug 592879.
- fix memleaks in test code (for the mochitest of this patch, and for the mochitest from the v0.3 patch in bug 592879).

The tryserver reported no failures, and no memleaks. Locally, in an opt build I get no failures, and no memleaks in a debug build.

I think this patch is ready to be checked-in, together with patch v0.3 from bug 592879.

Comment 57

[David Dahl :ddahl]

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

You ,don't need feedback if you are just rebasing

Comment 58

[David Dahl :ddahl]

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