651501 - document.body fails to autocomplete in Web Console

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

STR:

0. Open the Web Console on any web page.

1. type:
document.bo
in the jsterm.

2. see autocomplete "dy" (for document.body)

3. continue and type "dy.DO"

Expected result: you should have DOCUMENT ... etc ... as autocomplete (for the constants from body element).

Actual result: no autocomplete for any method/property in document.body.

Now repeat step 1 and you'll see that document.body is no longer autocompleted. It disappears from the list of possible completions.

Noticed this bug while working on the autocomplete popup code, bug 585991.

This bug affects Firefox 6 nightlies and Firefox 4 final as well.

Comment 1

[Mihai Sucan [:msucan]]

This happens with document.activeElement, document.childNodes and I believe many other properties. Can anyone else repro?

If yes, I think this bug needs a high priority fix, perhaps even a request to get the fix in Firefox 5 (aurora).

Comment 2

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

yeah, I can reproduce it. weird.

Comment 3

[Kevin Dangoor]

It may be early enough in Aurora to get a fix in if the fix is small and safe. If the fix is at all complex then it's going to be too risky for Aurora. The next train leaves in just a few weeks.

Comment 4

[Mihai Sucan [:msucan]]

Kevin: I think someone should be assigned to fix this bug and we'll see how risky it is once it's done.

At the moment I estimate the fix should be small and not too risky. We just need to find out the exact problem - I don't know it yet, bug needs investigation (which is 90% of the process of fixing it :) ).

Comment 5

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

This is caused because the document properties mentioned above (body, activeElement, childNodes) are all getter functions and there is a check in HUDService.jsm that skips those:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#3750

Should we not show them in the initial suggestion list as well?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to comment #5)
> This is caused because the document properties mentioned above (body,
> activeElement, childNodes) are all getter functions and there is a check in
> HUDService.jsm that skips those:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#3750
> 
> Should we not show them in the initial suggestion list as well?

I was expecting something like that, but as far as I know, the Firefox 4 code has no code to skip getters, and this bug applies to the Firefox 4 release as well. There must be something else going on.

Try removing the code that skips getters and see if document.body fails to autocomplete. If it fails, then it's not the getters we have to blame.

I would note that document.body disappears entirely from the Property Panel (Object Inspector). So, if it would become a getter, then it should show up in the inspector listed as "body: Getter". The weird part is that it doesn't show at all.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > This is caused because the document properties mentioned above (body,
> > activeElement, childNodes) are all getter functions and there is a check in
> > HUDService.jsm that skips those:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#3750
> > 
> > Should we not show them in the initial suggestion list as well?
> 
> I was expecting something like that, but as far as I know, the Firefox 4 code
> has no code to skip getters, and this bug applies to the Firefox 4 release as
> well. There must be something else going on.

I looked it up and apparently the check in question has been there since last July:

past@deck:devtools$ hg log -r 47924
changeset:   47924:d0f799546728
user:        Julian Viereck <[e-mail]>
date:        Mon Jul 19 11:38:07 2010 -0300
summary:     bug 568649 - JSTerm keybindings - tab completion. r=dietrich

It appears in the m-c FIREFOX_4_0b12_RELEASE tag and in what I presume to be the Firefox 4 repo:

http://mxr.mozilla.org/mozilla2.0/source/toolkit/components/console/hudservice/HUDService.jsm#3738

I also found a discussion of this check in the relevant bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=568649#c12

> Try removing the code that skips getters and see if document.body fails to
> autocomplete. If it fails, then it's not the getters we have to blame.

I removed it and then autocomplete works as expected (modulo the actual getter execution, I guess).

> I would note that document.body disappears entirely from the Property Panel
> (Object Inspector). So, if it would become a getter, then it should show up in
> the inspector listed as "body: Getter". The weird part is that it doesn't show
> at all.

It disappears indeed. I've also observed that if I inspect "document" before getting to one of the getter properties they subsequently disappear as well. That is, after the code finds out that a property is a getter, it eliminates it from the list. I haven't been able to see any property listed as "foo: Getter", yet. Do you have any examples I can test?

Comment 8

[Kevin Dangoor]

Does it make sense to skip over getters in the context of autocompletion?

In detail:

1. user types "doc", "document" object is found and the user expands to that
2. user types ".bo", "body" is found and user expands it to that. It doesn't matter that it's a getter, because the getter is not being accessed yet.
3. user types ".somethingThatExistsOnBody" - at this point, the getter for document.body must be accessed, but the user has signified that that's what they want to do...

We could always make a whitelist for certain getters, but it seems to me that we may not need to check for getters at all in the autocompletion context. (In the property panel, since we're displaying both the key and the value the getters would be activated without the user's explicit action to do so... it's a different case)

Comment 9

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

(In reply to comment #8)
> Does it make sense to skip over getters in the context of autocompletion?
> 
> In detail:
> 
> 1. user types "doc", "document" object is found and the user expands to that
> 2. user types ".bo", "body" is found and user expands it to that. It doesn't
> matter that it's a getter, because the getter is not being accessed yet.
> 3. user types ".somethingThatExistsOnBody" - at this point, the getter for
> document.body must be accessed, but the user has signified that that's what
> they want to do...
> 
> We could always make a whitelist for certain getters, but it seems to me
> that we may not need to check for getters at all in the autocompletion
> context. (In the property panel, since we're displaying both the key and the
> value the getters would be activated without the user's explicit action to
> do so... it's a different case)

I think this makes more sense than hiding it when checking for the ".bo" matches. It's the user who is in charge after all. Can anyone think of a Bad Thing(TM) that can happen when these getters are executed?

Comment 10

[Kevin Dangoor]

*some* getters shouldn't be executed randomly, but they won't have to be executed until the user tries to complete on the next level deep and the user will likely know which things they shouldn't execute (and most users won't have anything like that anyhow!)

Comment 11

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

Attachment: Disable the getter check

For testing purposes, this patch removes the getter check, so the aforementioned document properties autocomplete fine. I haven't been able to cause harm with it so far.

I'd still like to figure out why we get no "foo: Getter" properties in our property panel, though.

Comment 12

[Kevin Dangoor]

(In reply to comment #11)
> I'd still like to figure out why we get no "foo: Getter" properties in our
> property panel, though.

You mean the property panel should list the properties but automatically list their values? That's likely a good thought, and the object inspector in general could stand for some improvements.

Comment 13

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

(In reply to comment #9)
> I think this makes more sense than hiding it when checking for the ".bo"
> matches. It's the user who is in charge after all. Can anyone think of a Bad
> Thing(TM) that can happen when these getters are executed?

executing getters can create unintentional side-effects on the object you're inspecting.

I saw Dave Townsend file bug 656461 on this issue yesterday for Object literals displayed in the web console and I expect we're going to keep seeing it for fringe cases where some object get's mutated by having its getters fired. We don't want to unintentionally modify objects by looking at them. See Heisenberg.

(In reply to comment #12)
> You mean the property panel should list the properties but automatically
> list their values? That's likely a good thought, and the object inspector in
> general could stand for some improvements.

I think this is a bad idea for the reasons listed above. For DOM properties, it might be possible to build a sane whitelist but we're never going to be able to do that for userland objects.

See what Firebug does in the DOM panel for Getters.

Comment 14

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

Comment on attachment 531613 [details] [diff] [review]
Disable the getter check

I don't think we want to do this. We had good reason for putting that check in in the first place.

Could we complete the getter but then prevent further lookups on it -- possibly with an indicator that what you've completed is a Getter?

Comment 15

[Kevin Dangoor]

(In reply to comment #13)
> (In reply to comment #9)
> > I think this makes more sense than hiding it when checking for the ".bo"
> > matches. It's the user who is in charge after all. Can anyone think of a Bad
> > Thing(TM) that can happen when these getters are executed?
> 
> executing getters can create unintentional side-effects on the object you're
> inspecting.

My opinion is that autocompletion is a different case because the user has to explicitly type . to get to the next level down from the getter. Given the explicit user action here, I think that continuing to provide completion when asked is a good thing, even when it means calling a getter.

In Bespin, we used many getters that were like document.body: they just return an object and don't have side effects. Having completion there would be really nice.

> I saw Dave Townsend file bug 656461 on this issue yesterday for Object
> literals displayed in the web console and I expect we're going to keep
> seeing it for fringe cases where some object get's mutated by having its
> getters fired. We don't want to unintentionally modify objects by looking at
> them. See Heisenberg.

I absolutely agree that on display we don't want to automatically run getters. That can certainly lead to side effects. I definitely agree with Dave's bug there.


> (In reply to comment #12)
> > You mean the property panel should list the properties but automatically
> > list their values? That's likely a good thought, and the object inspector in
> > general could stand for some improvements.
> 
> I think this is a bad idea for the reasons listed above. For DOM properties,
> it might be possible to build a sane whitelist but we're never going to be
> able to do that for userland objects.

Sorry, I actually meant the exact opposite of what I said there. What I meant was that we should list the property names in the panel but put something like "Getter" for the value (and NOT automatically list their values) with some way for the user to expand them out. That is a separate thing from this bug, though.


Summary of my take on this particular bug: allow completion on the names of getters, allow completion of the properties on the result of the getters after the user has typed .

Comment 16

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

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #9)
> > > I think this makes more sense than hiding it when checking for the ".bo"
> > > matches. It's the user who is in charge after all. Can anyone think of a Bad
> > > Thing(TM) that can happen when these getters are executed?
> > 
> > executing getters can create unintentional side-effects on the object you're
> > inspecting.
> 
> My opinion is that autocompletion is a different case because the user has
> to explicitly type . to get to the next level down from the getter. Given
> the explicit user action here, I think that continuing to provide completion
> when asked is a good thing, even when it means calling a getter.

But that's the same action they do for any other property. I think we should have some indication that what they're doing is potentially different, whether that means highlighting or italicizing the getter or some other visual indication that what they're about to do is not the same "pressing ." that they usually do.

> In Bespin, we used many getters that were like document.body: they just
> return an object and don't have side effects. Having completion there would
> be really nice.

This isn't Bespin, and we shouldn't be inadvertently modifying content.

> > I saw Dave Townsend file bug 656461 on this issue yesterday for Object
> > literals displayed in the web console and I expect we're going to keep
> > seeing it for fringe cases where some object get's mutated by having its
> > getters fired. We don't want to unintentionally modify objects by looking at
> > them. See Heisenberg.
> 
> I absolutely agree that on display we don't want to automatically run
> getters. That can certainly lead to side effects. I definitely agree with
> Dave's bug there.

Likewise.

> > (In reply to comment #12)
> > > You mean the property panel should list the properties but automatically
> > > list their values? That's likely a good thought, and the object inspector in
> > > general could stand for some improvements.
> > 
> > I think this is a bad idea for the reasons listed above. For DOM properties,
> > it might be possible to build a sane whitelist but we're never going to be
> > able to do that for userland objects.
> 
> Sorry, I actually meant the exact opposite of what I said there. What I
> meant was that we should list the property names in the panel but put
> something like "Getter" for the value (and NOT automatically list their
> values) with some way for the user to expand them out. That is a separate
> thing from this bug, though.

ah, ok. Thanks for the clarification.

> Summary of my take on this particular bug: allow completion on the names of
> getters, allow completion of the properties on the result of the getters
> after the user has typed .

My take: Names of getters yes. Result afterwards should have some indicator or warning.

Comment 17

[Kevin Dangoor]

(In reply to comment #16)
> (In reply to comment #15)
> > My opinion is that autocompletion is a different case because the user has
> > to explicitly type . to get to the next level down from the getter. Given
> > the explicit user action here, I think that continuing to provide completion
> > when asked is a good thing, even when it means calling a getter.
> 
> But that's the same action they do for any other property. I think we should
> have some indication that what they're doing is potentially different,
> whether that means highlighting or italicizing the getter or some other
> visual indication that what they're about to do is not the same "pressing ."
> that they usually do.

OK, I'm fine with the idea of a visual indicator. The problem with some kind of highlight or italics is that the meaning of the change is not discoverable.

It may make sense to tie this into bug 585991 so that we can display something like (Getter) in the popup.

Comment 18

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

I've spent some time getting to the bottom of these issues and here is what I have discovered:

a) the reason we don't display document.{body,childNodes,activeElement} after the first time, is because we hit the getter check. Removing this check or replacing it with a looser one that allows native getters (which are assumed safe) lets these properties autocomplete. Such a loose check is used in the property panel code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/PropertyPanel.jsm#172

b) the reason we don't get "foo": Getter in the property panel is that we only show this for non-native ones.

c) we never execute getters (native or not) in the current code, nor in my patch (or my local one with the loose check).

d) if the user deliberately executes the getter, with say "document.body.ATTRIBUTE_NODE", we don't autocomplete the getter property from then on, but we do autocomplete its own properties. That is, after a getter execution, "document.bo" provides no suggestions, whereas "document.body." does. My theory is that this is a result of the native getter execution, for caching purposes. Initially, "body" is a property of the document's prototype chain, but after the execution it becomes a property of the document, shortening the access time. This would not be a bad thing, had the new cached property been marked as enumerable. Apparently this is not the case. Here is a web console session that supports this theory:

[19:27:39.906] for (let i in document) { if (i == "body") console.log("found it"); }
[19:27:39.916] found it @ Web Console:1
[19:27:39.922] undefined
--
[19:27:51.835] "body" in document
[19:27:51.845] true
--
[19:28:06.054] Object.getOwnPropertyNames(document).indexOf("body")
[19:28:06.067] -1
--
[19:28:16.857] document.body.ATTRIBUTE_NODE
[19:28:16.866] 2
--
[19:28:25.594] Object.getOwnPropertyNames(document).indexOf("body")
[19:28:25.604] 1
[19:28:30.544] "body" in document
[19:28:30.550] true
[19:28:33.209] for (let i in document) { if (i == "body") console.log("found it"); }
[19:28:33.220] undefined
--
[19:30:19.749] Object.getOwnPropertyDescriptor(document, "body")
[19:30:19.760] ({get:function body() {[native code]}, set:function body() {[native code]}, enumerable:false, configurable:true})

I've been trying to locate the actual native getter implementation to verify this, but all I've found is this which doesn't seem to be doing any caching:

http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1322


So, if my theory is correct, all of this boils down to two separate problems:

1) do we allow getter completion?
My take: yes, for both native and non-native, with a visual indicator, like Kevin suggests. The latter I believe should be done in bug 585991, or after that one lands, since they are apparently touching the same code.

2) what do we do about the lost ability to autocomplete getters after execution?
My take: we should be marking the cached properties as enumerable.

Comment 19

[Kevin Dangoor]

(In reply to comment #18)
> 
> 1) do we allow getter completion?
> My take: yes, for both native and non-native, with a visual indicator, like
> Kevin suggests. The latter I believe should be done in bug 585991, or after
> that one lands, since they are apparently touching the same code.
> 
> 2) what do we do about the lost ability to autocomplete getters after
> execution?
> My take: we should be marking the cached properties as enumerable.

Nice analysis, and I agree with your thoughts here (though clearly someone with more experience with the DOM implementation would need to speak up about whether there's a reason these properties are not enumerable).

The unfortunate thing is that I have no idea how many properties are in the same position as document.body. It would also be a shame to have the nice new autocompletion popup in Firefox 6 but not have any sort of solution to this.

What does Firebug do for this case?

Comment 20

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

I can no longer reproduce the (d) issue from comment 18 since changeset 38c72ce2dae4. I'm not sure what change exactly fixed it, but with attachment 531613 [details] [diff] [review] I now get the expected behavior (modulo the special indicator for getters).

Comment 21

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

Attachment: Disable only non-native getters

This is a different interim patch that only skips non-native getters, since the native ones are considered safe, as in the property panel code. It seems that with this patch I still get the (d) bug from comment 18, though. Mihai, do you get the same behavior with this as well?

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 533775 [details] [diff] [review]
Disable only non-native getters

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

Looks good, but I'd like some tests, to see how well this solves the problem we are having.

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +4206,5 @@
> +      // Check if prop is a non-native getter function on obj. Such functions
> +      // can change other stuff so we can't execute them to get the next
> +      // object. Stop here.
> +      let getter = obj.__lookupGetter__ ? obj.__lookupGetter__(prop) : null;
> +      if (getter && !isNativeFunction(getter)) {

I think it's safer to just do if (!isNativeFunction(getter)).

Comment 23

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

(In reply to comment #22)
> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +4206,5 @@
> > +      // Check if prop is a non-native getter function on obj. Such functions
> > +      // can change other stuff so we can't execute them to get the next
> > +      // object. Stop here.
> > +      let getter = obj.__lookupGetter__ ? obj.__lookupGetter__(prop) : null;
> > +      if (getter && !isNativeFunction(getter)) {
> 
> I think it's safer to just do if (!isNativeFunction(getter)).

This one actually doesn't work since the block would enter even when prop is not a getter at all. Moreover the whole patch doesn't work as I mentioned in comment 21, and I can see now why.

In a clean devtools working copy, load a page with the following code:

var OBJ = {
  get foo() {
    alert(1);
    delete OBJ.foo;
    return "bar";
  }
}

Now, try typing "OBJ.foo." in the web console, without pressing <ENTER>. No autocompletion follows, but no getter execution, either (you can verify this using <DEL> to see the foo suggestion again).

Now try typing "document.body.", again without <ENTER>. No autocompletion follows, but the native getter is executed, since you won't find "body" as a child of document again, as explained in comment 18. The existing getter check results in side-effects, even though the actual getter was never called! I'll attach a patch with dump()s that will make this clearer.

Is this a bug in platform code? It sure doesn't seem to be feasible to both check for native getters and not execute them. And if cached properties were marked as enumerable (as explained in comment 18) it wouldn't matter. One or the other has to happen though, if we are to fix this bug, as far as I can tell.

Comment 24

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

Attachment: The debugging patch mentioned in comment 23

Comment 25

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

Trying to find a workaround with Mihai we stumbled upon another issue. Using a "instaneof Ci.nsIDOMNode" before trying the __lookupGetter__ check works toavoid executing native getters, but causes non-native ones to fire! I'll attach the patch that we tried, which solves the problem with document.body disappearing, but causes "OBJ.foo." (from comment 23) to execute the getter. Since the alert() blocks the UI, we get a chance to verify that it's the instanceof check that fires it.

So, to summarize the situation, we have the main problem that document.__lookupGetter__("body") causes body to move from document's prototype to document itself, but marked as non-enumerable (even though OBJ.__lookupGetter__("foo") does nothing). And then, when we try to work around that, by only looking up the getter if (!(obj[prop] instanceof Ci.nsIDOMNode)), we get the OBJ.foo getter execution right there, in the instanceof check.

Comment 26

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

Attachment: The debugging patch mentioned in comment 25

After loading a page with a native getter like the one in comment 23, try typing "OBJ.foo." in the web console. You will witness the alert dialog right after "before intanceof" gets printed in stdout.

Comment 27

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

CCing mrbkap at dcamp's suggestion: is it a bug the behavior summarized in comment 25, that document.__lookupGetter__("body") causes body to move from document's prototype to document itself, but marked as non-enumerable? If so, should I open a platform bug for it?

Comment 28

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

Attachment: A working check

dcamp suggested a different way to check for getters that doesn't rely on __lookupGetter__, thus working for both native and non-native ones. As a matter of fact, native ones throw an error in getOwnPropertyDescriptor() for some reason unknown to me, but this is as good an indicator as any.

This fixes all of the errors reported in this bug.

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 535702 [details] [diff] [review]
A working check

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

The patch looks fine, and the solution works, indeed.

- Please make the same fixes to the PropertyPanel.jsm. document.body and other properties disappear once the user inspects the document object.
- Please add mochitests for the issues being fixed by this patch.

Thanks! Giving the patch r- until the comments are addressed. ;)

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +4153,5 @@
> +function isGetter(aScope, aObject, aProp) {
> +  let desc;
> +  while (aObject) {
> +    try {
> +      if (desc = aScope.Object.getOwnPropertyDescriptor(aObject, aProp)) {

Doesn't Object.getOwnPropertyDescriptor() work? Without aScope. If not, why not? Please explain.

@@ +4156,5 @@
> +    try {
> +      if (desc = aScope.Object.getOwnPropertyDescriptor(aObject, aProp)) {
> +        break;
> +      }
> +    } catch (e) {

Please use:
}
catch (ex) {

(as the rest of the code style)

@@ +4167,5 @@
> +    aObject = Object.getPrototypeOf(aObject);
> +  }
> +  if (!aObject) {
> +    return false;
> +  }

Is this check needed?

@@ +4168,5 @@
> +  }
> +  if (!aObject) {
> +    return false;
> +  }
> +  if (desc && desc.get) {

You want to add !isNativeFunction(desc.get) here as well. Some objects, like window.history, do not get autocomplete because they have getters and they do not throw in the while loop above. You need the isNativeFunction() check to allow execution of the native getters like window.history.

Please note that there's a bug in isIteratorOrGenerator() which also prevents window.history from showing autocomplete results (yay for double-bugs). The window.history object has a next() method that we check in the function (typeof obj.next). That throws an exception. Please wrap the function in a try-catch:

  try {
    if (typeof aObject.__iterator__ == "function" ||
        aObject.constructor && aObject.constructor.name == "Iterator") {
      return true;
    }

    let str = aObject.toString();
    if (typeof aObject.next == "function" &&
        str.indexOf("[object Generator") == 0) {
      return true;
    }
  }
  catch (ex) {
    return false;
  }

This fixes the code and makes window.history show autocomplete results. If you have a better fix for the issue at hand, please provide it in the patch. Thanks!

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #27)
> CCing mrbkap at dcamp's suggestion: is it a bug the behavior summarized in
> comment 25, that document.__lookupGetter__("body") causes body to move from
> document's prototype to document itself, but marked as non-enumerable? If
> so, should I open a platform bug for it?

It is a bug, and if you'd be kind enough to open one, I'd be more than happy to attach a patch to fix it.

Comment 31

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

(In reply to comment #30)
> (In reply to comment #27)
> > CCing mrbkap at dcamp's suggestion: is it a bug the behavior summarized in
> > comment 25, that document.__lookupGetter__("body") causes body to move from
> > document's prototype to document itself, but marked as non-enumerable? If
> > so, should I open a platform bug for it?
> 
> It is a bug, and if you'd be kind enough to open one, I'd be more than happy
> to attach a patch to fix it.

Thank you, I opened bug 660797 for this.

Regarding the "getter instanceof type"  and "typeof getter == type" checks mentioned in comment 25, is the getter execution there to be expected?

Comment 32

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

(In reply to comment #29)
> Comment on attachment 535702 [details] [diff] [review] [review]
> A working check
> 
> Review of attachment 535702 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> The patch looks fine, and the solution works, indeed.
> 
> - Please make the same fixes to the PropertyPanel.jsm. document.body and
> other properties disappear once the user inspects the document object.

I made the same changes and now document.body does not disappear from the property panel, but the non-native getters execute, unfortunately. This suggests that isGetter's scope parameter is not correct, although I can't see why. If you have any ideas I'd love to hear them, otherwise I'll investigate some more next week. Unsurprisingly this causes a relevant test to fail.

> - Please add mochitests for the issues being fixed by this patch.

Done.

> Thanks! Giving the patch r- until the comments are addressed. ;)
> 
> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +4153,5 @@
> > +function isGetter(aScope, aObject, aProp) {
> > +  let desc;
> > +  while (aObject) {
> > +    try {
> > +      if (desc = aScope.Object.getOwnPropertyDescriptor(aObject, aProp)) {
> 
> Doesn't Object.getOwnPropertyDescriptor() work? Without aScope. If not, why
> not? Please explain.

No it doesn't. This can be observed by removing aScope from this line and trying a getter on a host object, like comment 23. Typing "OBJ.foo." would execute the getter. I don't have a good theory of why getOwnPropertyDescriptor fails, even after taking a cursory look at the implementation (assuming I'm looking at the right place):

http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#1789

Figuring this out is what caused the about one hour delay between Dave's comment on IRC and my submitting the patch.

> @@ +4156,5 @@
> > +    try {
> > +      if (desc = aScope.Object.getOwnPropertyDescriptor(aObject, aProp)) {
> > +        break;
> > +      }
> > +    } catch (e) {
> 
> Please use:
> }
> catch (ex) {
> 
> (as the rest of the code style)

Fixed.

> @@ +4167,5 @@
> > +    aObject = Object.getPrototypeOf(aObject);
> > +  }
> > +  if (!aObject) {
> > +    return false;
> > +  }
> 
> Is this check needed?

Apparently not. I think it was a remnant from a version of the patch when I didn't return, but break out of the catch. Fixed.

> @@ +4168,5 @@
> > +  }
> > +  if (!aObject) {
> > +    return false;
> > +  }
> > +  if (desc && desc.get) {
> 
> You want to add !isNativeFunction(desc.get) here as well. Some objects, like
> window.history, do not get autocomplete because they have getters and they
> do not throw in the while loop above. You need the isNativeFunction() check
> to allow execution of the native getters like window.history.

Hmm, I wonder why some native getters throw while others don't. Anyway, fixed.

> Please note that there's a bug in isIteratorOrGenerator() which also
> prevents window.history from showing autocomplete results (yay for
> double-bugs). The window.history object has a next() method that we check in
> the function (typeof obj.next). That throws an exception. Please wrap the
> function in a try-catch:
> 
>   try {
>     if (typeof aObject.__iterator__ == "function" ||
>         aObject.constructor && aObject.constructor.name == "Iterator") {
>       return true;
>     }
> 
>     let str = aObject.toString();
>     if (typeof aObject.next == "function" &&
>         str.indexOf("[object Generator") == 0) {
>       return true;
>     }
>   }
>   catch (ex) {
>     return false;
>   }
> 
> This fixes the code and makes window.history show autocomplete results. If
> you have a better fix for the issue at hand, please provide it in the patch.
> Thanks!

Instead of a broad try/catch I added an explicit check for the history object, in order to avoid hiding any other unknown bugs. Let me know if you know of any other cases that would be missed by this check.

Comment 33

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

Attachment: Patch v2

Comment 34

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

Attachment: Patch v3

Fixed the aforementioned issue with executing getters in the property panel. All tests pass. One thing that still doesn't work is the display of window.history in the property panel, but it might be best to fix that in a separate bug, since it wasn't in the original bug report, and IMHO it would be good to land something soon for Aurora.

Comment 35

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

Attachment: Patch v4

OK, so I found that one, too. Inspecting the history property throws, so I made all such properties display [Inspection Error]. This way you can verify their existence, but still not continue peeking inside them.

Comment 36

[Mihai Sucan [:msucan]]

Comment on attachment 537547 [details] [diff] [review]
Patch v4

Good work! Patch is fine, r+!

We should have a test for the property panel fix as well. You are now only testing for the Web Console fix.

Possible concerns:

- I would prefer a try-catch around the failing typeof aObject.next instead of a check for the specific window history object. We don't know what other objects throw, and it's not really nice to check the window.history.
  - and you are assuming that currentContext().gBrowser.selectedBrowser.contentWindow is the global of aObject. That's not a very nice assumption.

- The try-catch clause would work for the property panel as well, which would avoid the need for [Inspection error].

- If we keep [Inspection error] I think we need this string localized - it's not an object type per-se, it's a message.

Comment 37

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

Attachment: Patch v5

(In reply to comment #36)
> Comment on attachment 537547 [details] [diff] [review] [review]
> Patch v4
> 
> Good work! Patch is fine, r+!
> 
> We should have a test for the property panel fix as well. You are now only
> testing for the Web Console fix.

Done.

> Possible concerns:
> 
> - I would prefer a try-catch around the failing typeof aObject.next instead
> of a check for the specific window history object. We don't know what other
> objects throw, and it's not really nice to check the window.history.
>   - and you are assuming that
> currentContext().gBrowser.selectedBrowser.contentWindow is the global of
> aObject. That's not a very nice assumption.

OK, reverted to the try/catch.

> - The try-catch clause would work for the property panel as well, which
> would avoid the need for [Inspection error].
> 
> - If we keep [Inspection error] I think we need this string localized - it's
> not an object type per-se, it's a message.

Removed the [Inspection Error] message and added a try/catch inside presentableValueFor().

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 537767 [details] [diff] [review]
Patch v5

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

Not entirely sure you can rely on native getters to not mutate anything but I guess I can't think of an example where one does so I guess this is ok. Got a few things that need tweaking here though.

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +4163,5 @@
> + *
> + * @return boolean
> + *         True if the given property is a getter, false otherwise.
> + */
> +function isGetter(aScope, aObject, aProp) {

You can probably use Components.utils.getGlobalForObject rather than passing the scope in.

@@ +4175,5 @@
> +    catch (ex) {
> +      // Native getters throw here:
> +      // [Exception... "Could not convert JavaScript argument"
> +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]

Rather than a copy of the exception could you include a comment about why they throw and a reference to a bug filed about it. It might also be worth checking that the exception is what you think it is.

@@ +4176,5 @@
> +      // Native getters throw here:
> +      // [Exception... "Could not convert JavaScript argument"
> +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]
> +      return false;

If native getters throw why isn't the result actually true?

@@ +4182,5 @@
> +    aObject = Object.getPrototypeOf(aObject);
> +  }
> +  if (desc && desc.get && !isNativeFunction(desc.get)) {
> +    return true;
> +  }

It looks like this function only returns true for non-native getters, update the name and comment to match please.

@@ +4302,5 @@
> +        return true;
> +      }
> +    }
> +    catch (ex) {
> +      return false;

When does this throw? Comment (and bug reference if appropriate) please

::: toolkit/components/console/hudservice/PropertyPanel.jsm
@@ +121,5 @@
> +            display: m[1]
> +          };
> +        }
> +      }
> +      catch (ex) {

When does this throw? Comment (and bug reference if appropriate) please

@@ +192,5 @@
> +  if (desc && desc.get && !isNativeFunction(desc.get)) {
> +    return true;
> +  }
> +  return false;
> +}

Same comments as previously. Is there not some shared devtools module that could hold this instead?

@@ +221,1 @@
>      // TODO: implement a safer way to skip non-native getters. See bug 647235.

Aren't we doing that here? Can this comment (and that bug) die?

@@ +221,3 @@
>      // TODO: implement a safer way to skip non-native getters. See bug 647235.
> +    let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +    let contentWindow = chromeWindow.gBrowser.selectedBrowser.contentWindow;

Using the window mediator feels a little concerning, any chance it'll give us the wrong window? Any reason not to use Components.utils.getGlobalForObject here?

Comment 39

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

Attachment: Patch v6

Thanks for the comments!

(In reply to comment #38)
> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +4163,5 @@
> > + *
> > + * @return boolean
> > + *         True if the given property is a getter, false otherwise.
> > + */
> > +function isGetter(aScope, aObject, aProp) {
> 
> You can probably use Components.utils.getGlobalForObject rather than passing
> the scope in.

Unfortunately it doesn't work. I get the same behavior like when not passing in the scope and just use Object.getOwnPropertyDescriptor. I don't have a good explanation on why it doesn't work, though.

> @@ +4175,5 @@
> > +    catch (ex) {
> > +      // Native getters throw here:
> > +      // [Exception... "Could not convert JavaScript argument"
> > +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> > +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]
> 
> Rather than a copy of the exception could you include a comment about why
> they throw and a reference to a bug filed about it. It might also be worth
> checking that the exception is what you think it is.

Done. I've added a check to make sure typeof aObject == "object" and rethrow the exception if it is not  NS_ERROR_XPC_BAD_CONVERT_JS (this is the case for document.body for instance) or NS_ERROR_XPC_BAD_OP_ON_WN_PROTO (this is the case for document.prefix for instance). These are all the cases that came up while running the console tests. 

I don't have a good answer on why these throw either. Furthermore, I have a hard time isolating this into a simple test case for a new bug. So I'm thinking that it would be better to file a bug after this one lands, that contains a patch against the repo that reproduces the error. I don't expect providing another patch against this patch is going to entice people to look into it, right?

> @@ +4176,5 @@
> > +      // Native getters throw here:
> > +      // [Exception... "Could not convert JavaScript argument"
> > +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> > +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]
> > +      return false;
> 
> If native getters throw why isn't the result actually true?
> 
> @@ +4182,5 @@
> > +    aObject = Object.getPrototypeOf(aObject);
> > +  }
> > +  if (desc && desc.get && !isNativeFunction(desc.get)) {
> > +    return true;
> > +  }
> 
> It looks like this function only returns true for non-native getters, update
> the name and comment to match please.

Yes, indeed. Fixed.

> @@ +4302,5 @@
> > +        return true;
> > +      }
> > +    }
> > +    catch (ex) {
> > +      return false;
> 
> When does this throw? Comment (and bug reference if appropriate) please
> 
> ::: toolkit/components/console/hudservice/PropertyPanel.jsm
> @@ +121,5 @@
> > +            display: m[1]
> > +          };
> > +        }
> > +      }
> > +      catch (ex) {
> 
> When does this throw? Comment (and bug reference if appropriate) please

Both this and the one above throw when checking window.history, because access to history.next is not allowed. Judging by this I think this is not a bug: https://developer.mozilla.org/en/DOM/window.history

> @@ +192,5 @@
> > +  if (desc && desc.get && !isNativeFunction(desc.get)) {
> > +    return true;
> > +  }
> > +  return false;
> > +}
> 
> Same comments as previously. Is there not some shared devtools module that
> could hold this instead?

There isn't one currently, but after bug 659907 lands HUDService.jsm will be importing PropertyPanel.jsm, so I just went ahead and imported it now, and the isNonNativeGetter & isNativeFunction functions are no longer duplicated.

> @@ +221,1 @@
> >      // TODO: implement a safer way to skip non-native getters. See bug 647235.
> 
> Aren't we doing that here? Can this comment (and that bug) die?

Yes, as discussed in IRC what we have here is good enough. Bug closed.

> @@ +221,3 @@
> >      // TODO: implement a safer way to skip non-native getters. See bug 647235.
> > +    let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser");
> > +    let contentWindow = chromeWindow.gBrowser.selectedBrowser.contentWindow;
> 
> Using the window mediator feels a little concerning, any chance it'll give
> us the wrong window? Any reason not to use
> Components.utils.getGlobalForObject here?

Unfortunately, as I mentioned previously getGlobalForObject doesn't work in all cases. But I can't really see how we could get the wrong window here, since this code runs after the user clicks on an object or executes inspect(object) in the console.

Comment 40

[Dave Townsend [:mossop]]

(In reply to comment #39)

Couple of reply comments here, will do an actual review of the new patch hopefully later today.

> (In reply to comment #38)
> > ::: toolkit/components/console/hudservice/HUDService.jsm
> > @@ +4163,5 @@
> > > + *
> > > + * @return boolean
> > > + *         True if the given property is a getter, false otherwise.
> > > + */
> > > +function isGetter(aScope, aObject, aProp) {
> > 
> > You can probably use Components.utils.getGlobalForObject rather than passing
> > the scope in.
> 
> Unfortunately it doesn't work. I get the same behavior like when not passing
> in the scope and just use Object.getOwnPropertyDescriptor. I don't have a
> good explanation on why it doesn't work, though.

Strange, can you file a bug on that please.

> > @@ +4175,5 @@
> > > +    catch (ex) {
> > > +      // Native getters throw here:
> > > +      // [Exception... "Could not convert JavaScript argument"
> > > +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> > > +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]
> > 
> > Rather than a copy of the exception could you include a comment about why
> > they throw and a reference to a bug filed about it. It might also be worth
> > checking that the exception is what you think it is.
> 
> Done. I've added a check to make sure typeof aObject == "object" and rethrow
> the exception if it is not  NS_ERROR_XPC_BAD_CONVERT_JS (this is the case
> for document.body for instance) or NS_ERROR_XPC_BAD_OP_ON_WN_PROTO (this is
> the case for document.prefix for instance). These are all the cases that
> came up while running the console tests. 
> 
> I don't have a good answer on why these throw either. Furthermore, I have a
> hard time isolating this into a simple test case for a new bug. So I'm
> thinking that it would be better to file a bug after this one lands, that
> contains a patch against the repo that reproduces the error. I don't expect
> providing another patch against this patch is going to entice people to look
> into it, right?

Sounds fine, just reference the bug in the comment.

> > ::: toolkit/components/console/hudservice/PropertyPanel.jsm
> > @@ +121,5 @@
> > > +            display: m[1]
> > > +          };
> > > +        }
> > > +      }
> > > +      catch (ex) {
> > 
> > When does this throw? Comment (and bug reference if appropriate) please
> 
> Both this and the one above throw when checking window.history, because
> access to history.next is not allowed. Judging by this I think this is not a
> bug: https://developer.mozilla.org/en/DOM/window.history

Accessing the value of the property should throw yes, I guess I don't know if accessing the property descriptor for the property should throw too. And aren't we considered chrome code? It might be worth double checking this with someone on the JS team, but I'm happy with just a comment on what sorts of cases would cause these to throw.

Comment 41

[Dave Townsend [:mossop]]

Comment on attachment 538499 [details] [diff] [review]
Patch v6

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

Add the comments and requested bugs as above but this looks fine. Sorry for the delay

Comment 42

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

Attachment: [in-devtools] Patch v7

(In reply to comment #40)
> > (In reply to comment #38)
> > > ::: toolkit/components/console/hudservice/HUDService.jsm
> > > @@ +4163,5 @@
> > > > + *
> > > > + * @return boolean
> > > > + *         True if the given property is a getter, false otherwise.
> > > > + */
> > > > +function isGetter(aScope, aObject, aProp) {
> > > 
> > > You can probably use Components.utils.getGlobalForObject rather than passing
> > > the scope in.
> > 
> > Unfortunately it doesn't work. I get the same behavior like when not passing
> > in the scope and just use Object.getOwnPropertyDescriptor. I don't have a
> > good explanation on why it doesn't work, though.
> 
> Strange, can you file a bug on that please.

Filed bug 664426.

> > > @@ +4175,5 @@
> > > > +    catch (ex) {
> > > > +      // Native getters throw here:
> > > > +      // [Exception... "Could not convert JavaScript argument"
> > > > +      // nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"
> > > > +      // location: "JS frame :: resource:///modules/HUDService.jsm :: isGetter :: line 4189"  data: no]
> > > 
> > > Rather than a copy of the exception could you include a comment about why
> > > they throw and a reference to a bug filed about it. It might also be worth
> > > checking that the exception is what you think it is.
> > 
> > Done. I've added a check to make sure typeof aObject == "object" and rethrow
> > the exception if it is not  NS_ERROR_XPC_BAD_CONVERT_JS (this is the case
> > for document.body for instance) or NS_ERROR_XPC_BAD_OP_ON_WN_PROTO (this is
> > the case for document.prefix for instance). These are all the cases that
> > came up while running the console tests. 
> > 
> > I don't have a good answer on why these throw either. Furthermore, I have a
> > hard time isolating this into a simple test case for a new bug. So I'm
> > thinking that it would be better to file a bug after this one lands, that
> > contains a patch against the repo that reproduces the error. I don't expect
> > providing another patch against this patch is going to entice people to look
> > into it, right?
> 
> Sounds fine, just reference the bug in the comment.

Done. I've found bug 520882. This appears to be a known issue.

> > > ::: toolkit/components/console/hudservice/PropertyPanel.jsm
> > > @@ +121,5 @@
> > > > +            display: m[1]
> > > > +          };
> > > > +        }
> > > > +      }
> > > > +      catch (ex) {
> > > 
> > > When does this throw? Comment (and bug reference if appropriate) please
> > 
> > Both this and the one above throw when checking window.history, because
> > access to history.next is not allowed. Judging by this I think this is not a
> > bug: https://developer.mozilla.org/en/DOM/window.history
> 
> Accessing the value of the property should throw yes, I guess I don't know
> if accessing the property descriptor for the property should throw too. And
> aren't we considered chrome code? It might be worth double checking this
> with someone on the JS team, but I'm happy with just a comment on what sorts
> of cases would cause these to throw.

Added comments and sent a post to mozilla.dev.tech.js-engine:

http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/b5aa5d462ef66e89#


Thanks for the review!

Comment 43

[Dave Camp (:dcamp)]

Comment on attachment 539554 [details] [diff] [review]
[in-devtools] Patch v7

http://hg.mozilla.org/projects/devtools/rev/cbc782458ff1

Comment 44

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

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

Comment 45

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

backed out of mozilla-central due to test failure.

http://hg.mozilla.org/mozilla-central/rev/68253b0ab50d

Please update the unittest to not use hard-coded numbers of properties.

Comment 46

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

Attachment: Patch v8

Fixed the test to not use a hard-coded number of document properties.

Comment 47

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

http://hg.mozilla.org/projects/devtools/rev/3f4b311528c4

Comment 48

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

http://hg.mozilla.org/mozilla-central/rev/3f4b311528c4

Comment 49

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110630 Firefox/7.0a1

Verified isse on Ubuntu 11.04, WinXP, Mac OS X 10.6 and Win 7 using the steps fromm Comment 0.

Issue no longer reproducible -> setting status to Verified Fixed.