943496 - Autocomplete should execute native getters

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mihai Sucan [:msucan]]

document.title returns a getter and completion suggestions are inaccurate. We need to execute safe/native getters.

Comment 1

[Mihai Sucan [:msucan]]

Regression caused by bug 842682. We have a test that was supposed to check this stuff, browser_webconsole_bug_651501_document_body_autocomplete.js. However, it changed too much in time and now it's too permissive - it doesn't check which properties are shown in the popup.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: bug-943496-1-fx-team.diff

Patch started as a bit of code shuffle in JSPropertyProvider - it grew too confusing / easy to break. Then I found the problem with executing native getters - see DebuggerObjectSupport.getProperty().

Any improvements we could make?

In the future, it would be nice if we could reuse stuff from actors/script.js. I saw we have functions there for similar purposes.

(no try push yet, due to tree closure)

Comment 3

[Mihai Sucan [:msucan]]

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

Comment 4

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

Comment on attachment 8338831 [details] [diff] [review]
bug-943496-1-fx-team.diff

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

Looks fine to me, but it would be better to add an explicit test for document.title or a similar getter.

::: toolkit/devtools/webconsole/utils.js
@@ +1037,5 @@
>  
> +    let safe = desc.get.callable && desc.get.class == "Function" &&
> +               desc.get.script === undefined;
> +    if (!safe) {
> +      return null;

Maybe we should move the hasSafeGetter() and getProperty() helper functions from script.js to DevToolsUtils.js and reuse them here, but it's up to you.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: bug-943496-2-fx-team.diff

Thanks for the review!

Updated the patch to move getProperty() and hasSafeGetter() as suggested. Also added testing for document.title.

Do note that I fixed two issues in getProperty(): (1) it called native getter functions on prototype objects, not on the root/start object, which made it fail in many cases; (2) when it executed a native getter it returned a debugger completion value, not the actual function return value - breaking its callers. I found these problems when working on bug 843004.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=7b7c7340f503

Comment 6

[Mihai Sucan [:msucan]]

Attachment: bug-943496-2-interdiff.diff

bonus interdiff :)

Comment 7

[Mihai Sucan [:msucan]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7f5c3b76b29