Closed Bug 1169096 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Object Inspector, defect)

40 Branch
defect
Not set
normal

Tracking

(firefox40 unaffected, firefox41+ fixed, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: bgrins, Assigned: ochameau)

References

Details

(Keywords: regression)

Attachments

(4 files, 7 obsolete files)

Attached image 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)
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
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!
(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..
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.
Keywords: regression
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.
Attached patch patch v1 (obsolete) — Splinter Review
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!
And here is a patch to filter property values.
(without that, we were just looking at property names)
Assignee: nobody → poirot.alex
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)
(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: → 1171723
Status: NEW → ASSIGNED
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+
Attachment #8614130 - Attachment is obsolete: true
Attachment #8614671 - Attachment is obsolete: true
Attached patch Test attribute filtering - v1 (obsolete) — Splinter Review
And a test that covers both patches.
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
Attachment #8628802 - Flags: review?(past)
Attachment #8617388 - Flags: review?(past)
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+
(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.
Attached patch Test attribute filtering - v2 (obsolete) — Splinter Review
Attachment #8617388 - Attachment is obsolete: true
Attachment #8631539 - Flags: review+
Same patch with macos 10.10 run, to see if that reproduces on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf20620033
Another try with additional tweaks that may fix such failure:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1f02df54c8
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!
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
Attached patch interdiff (obsolete) — Splinter Review
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
Attachment #8636571 - Flags: review?(bgrinstead)
Attachment #8636571 - Flags: review?(bgrinstead) → review+
Attachment #8631539 - Attachment is obsolete: true
Attachment #8636571 - Attachment is obsolete: true
Attachment #8636764 - Flags: review+
[Tracking Requested - why for this release]:
Consider uplift to repair object filtering in DevTools
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)
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)
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?
Attachment #8628802 - Flags: approval-mozilla-aurora?
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+
See Also: → 1214219
See Also: 1214219
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: