Autocomplete should execute native getters

RESOLVED FIXED in Firefox 28

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

({regression})

Trunk
Firefox 28
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

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

Comment 1

6 years ago
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
Assignee

Comment 2

6 years ago
Posted 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+
Assignee

Comment 5

6 years ago
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
Assignee

Comment 6

6 years ago
bonus interdiff :)
https://hg.mozilla.org/mozilla-central/rev/e7f5c3b76b29
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Assignee

Updated

5 years ago
Duplicate of this bug: 946404

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.