Filter not working for object inspection with nested properties in variables view

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bgrins, Assigned: ochameau)

Tracking

({regression})

40 Branch
Firefox 42
regression
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 unaffected, firefox41+ fixed, firefox42 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

84.56 KB, image/png
Details
2.13 KB, patch
past
: review+
Details | Diff | Splinter Review
8.95 KB, patch
past
: review+
Details | Diff | Splinter Review
4.00 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8611582 [details]
filter-not-working.png

I believe this is due to Bug 1023386 since it's working in nightly but not a recent build:

STR:

console.log({foo: {bar: {baz: 1}}})
Click on the Object link
Expand the properties
Search for 'baz'

Expected:

'baz' is visible

Actual

Nothing is found.  Also, oddly the text  'Object' is replaced with the string that was searched for (see screenshot)
(Assignee)

Comment 1

3 years ago
Oh yes, that's because of bug 1023386. I didn't realized filtering was recursive :/
That makes it much more complex!
Whenever you search for something, it recompute a new variable tree and reset what you already expanded. And, it only search for properties in the top level object...
Also I just realized, the new filter only searches for property names, whereas the previous search was also looking at property preview string.

Regarding the 'Object' text, that's much simplier story.
It is defined here, the addScope argument is the title:
  https://hg.mozilla.org/integration/fx-team/file/07cc8095956b/browser/devtools/shared/widgets/VariablesView.jsm#l554
(Assignee)

Comment 2

3 years ago
I can remove the remote filtering feature, that will make that work more like before.
i.e. it will only be able to filter, but deeply, whatever has been expanded.
Except that now, we do not show all the properties if the object is big...

Implementing it with the exact same behaviour, with nice performances with remote target (e10s!), sounds like a quite significant work!
(Reporter)

Comment 3

3 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Oh yes, that's because of bug 1023386. I didn't realized filtering was
> recursive :/
> That makes it much more complex!
> Whenever you search for something, it recompute a new variable tree and
> reset what you already expanded. And, it only search for properties in the
> top level object...

I don't think searching for properties in the top level object only is desirable behavior, but apparently that is how it previously worked.  Ideally filtering would always work as expected (i.e. include any nested properties / values regardless of expansion).  I'm not sure if that makes this easier or harder..
(Assignee)

Comment 4

3 years ago
Searching for properties names and values regarless of expansion is something we can't do.
Imagine doing that on the window object... it is like searching in almost everything!
We could do such deep search, but only partially, with a reasonable limit.
Searching in nested that has been expanded is one way to do it, but it gives the feeling that we are searching on everything whereas we aren't.
(Reporter)

Updated

3 years ago
Keywords: regression
(Reporter)

Comment 5

3 years ago
We've discussed a possible hybrid approach where the server returns all top level results (even non loaded) and the client adds in results from visible expanded properties.  Currently the first part of this is already working.  The issue is right now is that the client is the only place that knows which tree objects have been expanded.
(Assignee)

Comment 6

3 years ago
Created attachment 8614130 [details] [diff] [review]
patch v1

Wasn't that complex if we make the assumption that already existing attributes
aren't duplicated... but I may have missed some corner cases!

There is at least this one:
  let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a)

Try to search for 100 and then reset the filter,
you will end up with a mixed view of regular splits, plus the search results.
I'm not sure that's really bad, it just looks living!
(Assignee)

Comment 7

3 years ago
Created attachment 8614671 [details] [diff] [review]
Filter property values remotely - v1

And here is a patch to filter property values.
(without that, we were just looking at property names)
(Assignee)

Updated

3 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 8

3 years ago
brian, what do you think of these two patches? Do you feel reviewing them or should I ask Panos to take a look?
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 9

3 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Created attachment 8614130 [details] [diff] [review]
> patch v1
> 
> Wasn't that complex if we make the assumption that already existing
> attributes
> aren't duplicated... but I may have missed some corner cases!
> 
> There is at least this one:
>   let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a)
> 
> Try to search for 100 and then reset the filter,
> you will end up with a mixed view of regular splits, plus the search results.
> I'm not sure that's really bad, it just looks living!

Thanks for the patches.  Two things I noticed:

1) When I reset the filter with the clear button it isn't clearing out the results (I have to press enter once it's already cleared out or ESC to get it to work.  This appears to be a separate bug though - I've filed Bug 1171723 for this
2) Having the mixed in loaded results after clearing is a little confusing.  Luckily the results still show up in the splits as I'd expect so it's not too bad.  But how hard it would be to remove these from the results once the search has been cleared - so things go back to how they were before the search?
Flags: needinfo?(bgrinstead)
See Also: → bug 1171723
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 10

3 years ago
Comment on attachment 8614130 [details] [diff] [review]
patch v1

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

Looking through the patches, I think it'd be best to have Panos review this code since I'm not as familiar with the VariablesView.  We will surely need tests for both of these changes, but flagging for initial feedback to start
Attachment #8614130 - Flags: feedback?(past)
Comment on attachment 8614130 [details] [diff] [review]
patch v1

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +556,3 @@
>        this.controller.performSearch(scope, aToken);
> +      // Filter already displayed attributes
> +      scope._performSearch(aToken.toLowerCase());

Don't we need to treat null, undefined and empty string specially here, too?
Attachment #8614130 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 12

3 years ago
Created attachment 8617384 [details] [diff] [review]
Merge local and remote results - v2
Attachment #8614130 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8617386 [details] [diff] [review]
Filter property values remotely - v2
Attachment #8614671 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8617388 [details] [diff] [review]
Test attribute filtering - v1

And a test that covers both patches.
(Assignee)

Comment 15

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd510ccae06

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Alexandre Poirot [:ochameau] from comment #6)
> > Created attachment 8614130 [details] [diff] [review]
> > patch v1
> > 
> > Wasn't that complex if we make the assumption that already existing
> > attributes
> > aren't duplicated... but I may have missed some corner cases!
> > 
> > There is at least this one:
> >   let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a)
> > 
> > Try to search for 100 and then reset the filter,
> > you will end up with a mixed view of regular splits, plus the search results.
> > I'm not sure that's really bad, it just looks living!
> 
> Thanks for the patches.  Two things I noticed:
> 
> 1) When I reset the filter with the clear button it isn't clearing out the
> results (I have to press enter once it's already cleared out or ESC to get
> it to work.  This appears to be a separate bug though - I've filed Bug
> 1171723 for this

Submitted a patch there.

> 2) Having the mixed in loaded results after clearing is a little confusing. 
> Luckily the results still show up in the splits as I'd expect so it's not
> too bad.  But how hard it would be to remove these from the results once the
> search has been cleared - so things go back to how they were before the
> search?

I did that in new patch, the result is cleared when we clear the filter box completely.
We still have mixed results if we just remove some characters.

I also added a test in a 3rd patch.
Status: ASSIGNED → NEW
(Assignee)

Comment 16

3 years ago
Created attachment 8627665 [details] [diff] [review]
Merge local and remote results - v3

Fixed the failing test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7308c2d7dacb
Attachment #8617384 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 17

3 years ago
Created attachment 8628802 [details] [diff] [review]
Merge local and remote results - v4

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6947b934775f
Attachment #8627665 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8628802 - Flags: review?(past)
(Assignee)

Updated

3 years ago
Attachment #8617388 - Flags: review?(past)
(Assignee)

Updated

3 years ago
Attachment #8617386 - Flags: review?(past)
Attachment #8617386 - Flags: review?(past) → review+
Comment on attachment 8617388 [details] [diff] [review]
Test attribute filtering - v1

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

::: browser/devtools/webconsole/test/browser_console_variables_view_filter.js
@@ +81,5 @@
> +  view = yield fetched;
> +
> +  assertAttrs(view, "foo", "If we reset again, we get back to original state again");
> +
> +  gWebConsole = gJSTerm = gVariablesView = null;

There is also |hud|, but why do you need all these variables in the global scope if you are only accessing them in the test function?
Attachment #8617388 - Flags: review?(past) → review+
Attachment #8628802 - Flags: review?(past) → review+
(Assignee)

Comment 19

3 years ago
(In reply to Panos Astithas [:past] from comment #18)
> Comment on attachment 8617388 [details] [diff] [review]
> Test attribute filtering - v1
> 
> Review of attachment 8617388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/test/browser_console_variables_view_filter.js
> @@ +81,5 @@
> > +  view = yield fetched;
> > +
> > +  assertAttrs(view, "foo", "If we reset again, we get back to original state again");
> > +
> > +  gWebConsole = gJSTerm = gVariablesView = null;
> 
> There is also |hud|, but why do you need all these variables in the global
> scope if you are only accessing them in the test function?

I just copy pasted some existing test... I refactored this to use local-scoped variables.
(Assignee)

Comment 20

3 years ago
Created attachment 8631539 [details] [diff] [review]
Test attribute filtering - v2
Attachment #8617388 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8631539 - Flags: review+
(Assignee)

Comment 25

3 years ago
Same patch with macos 10.10 run, to see if that reproduces on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf20620033
(Assignee)

Comment 26

3 years ago
Another try with additional tweaks that may fix such failure:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1f02df54c8
(Assignee)

Comment 27

3 years ago
Ok, so this is an intermittent, that only reproduces during high load on slaves...
The good news is that the additional tweaks seems to fix it.
The bad news is that I see at least one another intermittent, reproduced only once for now:
  browser/devtools/webconsole/test/browser_console_variables_view_filter.js | If we don't manually expand nested attr, we don't see them in search - Got foo, expected

I just launched some more runs to see if I can see it being reproduced more than just once!
(Assignee)

Comment 28

3 years ago
Looks like a real intermittent, it has been reproduced twice.
MacOS seems to be sensible on Command event being dispatched on xul:textbox...

Let's try another approach to write this test:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97436c93adb
(Assignee)

Comment 29

3 years ago
Created attachment 8636571 [details] [diff] [review]
interdiff

Here is how to fix intermittent on Mac.
i.e. do not rely on asynchronous code for firing command event from textbox.xml (XBL)...
This is an interdiff on top of attachment 8631539 [details] [diff] [review].

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97436c93adb
(Assignee)

Updated

3 years ago
Attachment #8636571 - Flags: review?(bgrinstead)
(Reporter)

Updated

3 years ago
Attachment #8636571 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 31

3 years ago
Created attachment 8636764 [details] [diff] [review]
Test attribute filtering - v3
Attachment #8631539 - Attachment is obsolete: true
Attachment #8636571 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8636764 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/63afe6a4010a
https://hg.mozilla.org/mozilla-central/rev/71c1cea7ff97
https://hg.mozilla.org/mozilla-central/rev/b8787cf5a4d3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
[Tracking Requested - why for this release]:
Consider uplift to repair object filtering in DevTools
status-firefox40: --- → unaffected
status-firefox41: --- → affected
tracking-firefox41: --- → ?
Tracked.
tracking-firefox41: ? → +
Flags: needinfo?(jryans)
Ryan, could you please request uplift to Aurora whenever the patch seems stable and ready?
I don't know if the landed patches are the same as the bug attachments, so I'll leave to Alex to request uplift.
Flags: needinfo?(jryans) → needinfo?(poirot.alex)
(Assignee)

Comment 37

3 years ago
Try on aurora looks good:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ecfd75f2c9
But the patches may not apply as-is on aurora.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 38

3 years ago
Comment on attachment 8617386 [details] [diff] [review]
Filter property values remotely - v2

Approval Request Comment
[Feature/regressing bug #]:
  Fixing regression from bug 1023386 
[User impact if declined]: 
  Different variable inspector behavior between releases
[Describe test coverage new/current, TreeHerder]:
  introduce new test covering this regressed feature
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8617386 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8628802 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8636764 - Flags: approval-mozilla-aurora?
Comment on attachment 8617386 [details] [diff] [review]
Filter property values remotely - v2

Approved as this has been in m-c for a while and seems safe to uplift to Aurora.
Attachment #8617386 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8628802 [details] [diff] [review]
Merge local and remote results - v4

Approved.
Attachment #8628802 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8636764 [details] [diff] [review]
Test attribute filtering - v3

Approved.
Attachment #8636764 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
See Also: → bug 1214219

Updated

3 years ago
See Also: bug 1214219

Updated

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