Closed Bug 943496 Opened 7 years ago Closed 7 years ago

Autocomplete should execute native getters

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: msucan, Assigned: msucan)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

document.title returns a getter and completion suggestions are inaccurate. We need to execute safe/native getters.
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.
Depends on: 842682
Keywords: regression
Attached patch bug-943496-1-fx-team.diff (obsolete) — Splinter Review
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)
Attachment #8338831 - Flags: review?(past)
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.
Attachment #8338831 - Flags: review?(past) → review+
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
Attachment #8338831 - Attachment is obsolete: true
bonus interdiff :)
https://hg.mozilla.org/mozilla-central/rev/e7f5c3b76b29
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Duplicate of this bug: 946404
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.