1023386 - Inspecting 'window' crashes the browser for complex pages

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[Joe Walker [:jwalker]]

STR:

1. View a Google Docs document.
2. Open the console
3. Type 'window' at the JS prompt
4. Click on the 'window' link to open the VariablesView
5. Observe lack of response
6. Wait for 'slow script' dialog
7. Attempt to regain control of browser
8. Give up and force quit

Also relevant:

> Object.getOwnPropertyNames(window).length
18848

That's probably got something to do with it.

Comment 1

[Brian Grinstead [:bgrins]]

Inspecting the test case from bug 998344 also causes a hang:

> new Uint8Array(new ArrayBuffer(2354207));
Click on result in console
 
Or a more manageable size just to show the lag without crashing

> new Uint8Array(new ArrayBuffer(30000));

Comment 2

[Alexandre Poirot [:ochameau]]

I'll try to look into this one.

Comment 3

[Alexandre Poirot [:ochameau]]

There is some split by four sub-items if the array is bigger than 500 items.
But unfortunately, that's only made on client that. Which means we create an object actor for each item anyway.
We should do something at actor/server level. And ideally, not just for array, for also objet with (too) many attributes.

Comment 4

[Alexandre Poirot [:ochameau]]

Note that Chrome is smart for (typed) arrays, but not for object with many properties.
It hangs when inspecting `window[0]` on gmail (logged in, on inbox page), that contains tons of globals.

The original report is about slowness when inspecting objects with many named attributes,
while comment 1 is about array-like objects.

I'm currently trying to craft a patch to cover both usages but covering the first usecase is more complex,
as, in our case, we have many "safe getter" being displayed for window object. Also trying to split/sort named attributes may require more UX thoughts. Or is the following display easy to understand:

  Window:
    [alert..onclick]
      alert: function () {}
      ...
      ...
      onclick: function() {}
    [oncontextmenu..sizeToContent]
      oncontextmenu: function () {}
      ...
      ...
      sizeToContent: function () {}

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Here is a first batch, with tests!
It splits the big arrays, like today in Fx (but only non-e10s) and Chrome.
But it also splits the objects with many properties, like the window 
objects with many global objects.

I could have hacked something simplier to support only big arrays,
but thought we could do better on various usecases...

Feedback welcomed!!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=008fa9ba16fa

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8598723 [details] [diff] [review]
patch v1

Starting with a feedback request as this change is quite significant in term of UX and actor code...
Panos, I flagged you as I've seen your name in blame, but feel free to involve someone else.

You can play with this patch against:
  new Uint8Array(new ArrayBuffer(30000));
or just:
  window
or a mixed usecase like:
  var a = new Uint8Array(new ArrayBuffer(1000)); a.aaa=999; a

Comment 7

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

Comment on attachment 8598723 [details] [diff] [review]
patch v1

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

This is a significant change to how we handle the object actor, but I can't see anything wrong with the approach. I will spend more time on it when it's ready for review. In the meantime I spotted a few things while reading.

::: toolkit/devtools/client/dbg-client.jsm
@@ +2241,5 @@
> +        return { iterator: new PropertyIteratorClient(this._client, aResponse.iterator) };
> +      }
> +      return aResponse;
> +    },
> +    telemetry: "PROTOTYPESANDPROPERTIES"

ENUMPROPERTIES

@@ +2307,5 @@
>   *        The debugger client parent.
>   * @param aGrip Object
>   *        A pause-lifetime long string grip returned by the protocol.
>   */
> +function PropertyIteratorClient(aClient, aGrip) {

The comment here is wrong.

::: toolkit/devtools/server/actors/root.js
@@ +168,5 @@
>      },
>      // Whether or not `getProfile()` supports specifying a `startTime`
>      // and `endTime` to filter out samples. Fx40+
>      profilerDataFilterable: true,
> +    // ThreadActors supports fetching only a subset of properties

Typo: ThreadActor supports fetching only a subset of available properties

@@ +169,5 @@
>      // Whether or not `getProfile()` supports specifying a `startTime`
>      // and `endTime` to filter out samples. Fx40+
>      profilerDataFilterable: true,
> +    // ThreadActors supports fetching only a subset of properties
> +    supportsPropertySlices: true,

It seems you found a better way to feature-test by testing the object actor, right?

::: toolkit/devtools/server/actors/script.js
@@ +3284,5 @@
> +
> +/**
> + * Creates an actor to iterate over an object properties.
> + *
> + * @param aObjectActor Debugger.Object

aObjectActor is of the ObjectActor type, not Debugger.Object.

@@ +3287,5 @@
> + *
> + * @param aObjectActor Debugger.Object
> + *        The debuggee object.
> + * @param aOptions ThreadActor
> + *        The parent thread actor for this object.

aOptions is not a ThreadActor.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79b03ded458

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8602783 [details] [diff] [review]
patch v2

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

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +160,5 @@
>    },
>  
> +  _populatePropertySlices: function(aTarget, aGrip, aIterator) {
> +    // Divide the keys into quarters.
> +    let items = Math.ceil(aGrip.ownPropertyLength / 4);

Some of this is also handled in the actual variables view [0]. This is a much better place to do it though, but we really really absolutely shouldn't do this in two different places.

[0] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm?from=variablesview.jsm#1330

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #10)
> 
> This is a much better place to do it though

And by "this" I mean the controller.

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79b03ded458

Is this Linux only? I would love to give UX feedback, and would love to have a mac build in order to do so, eventually.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Jeff Griffiths (:canuckistani) from comment #12)
> (In reply to Alexandre Poirot [:ochameau] from comment #9)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79b03ded458
> 
> Is this Linux only? I would love to give UX feedback, and would love to have
> a mac build in order to do so, eventually.

No, this is just to save ressources. I just looking for test failures and they usually happen on any platform.
Here is a try for a mac build:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9276917167

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I took a look and I think the idea is reasonable for very large objects. My one problem is, this patch happens for all windows. Bugzilla for example has 842 properties on window and we can probably just print it with acceptable perf. I think we should identify a minimum # of properties and engage this approach at that point. 2000?

The other problem is the filter does not work until a particular tree has been expanded, so it's kind of useless without a bunch of initial clicking.

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> (In reply to Victor Porof [:vporof][:vp] from comment #10)
> > 
> > This is a much better place to do it though
> 
> And by "this" I mean the controller.

Should I just drop the similar code from VariableView.jsm? 
As I highlighted during last meeting, we have a bunch of code to drop if we switch to pure-e10s and ignore local codepath with direct access to object.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Fixed existing tests and improved handling of arrays with attributes.
By splitting first array items and displaying named attributes after the splits.
(It will also split named attributes if there is many)

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Jeff Griffiths (:canuckistani) from comment #14)
> I took a look and I think the idea is reasonable for very large objects. My
> one problem is, this patch happens for all windows. Bugzilla for example has
> 842 properties on window and we can probably just print it with acceptable
> perf. I think we should identify a minimum # of properties and engage this
> approach at that point. 2000?
> 
> The other problem is the filter does not work until a particular tree has
> been expanded, so it's kind of useless without a bunch of initial clicking.

Thanks for the feedback Jeff!
I've updated the threshold to 2000 in my last patch. But it may be high for slow hardware on e10s...
That's something we can easily tune in followups.
Also, the filter issue is really painful, but I'm wondering if I should block on it, it sounds like a quite significant work as well.

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> Also, the filter issue is really painful, but I'm wondering if I should
> block on it, it sounds like a quite significant work as well.

It seems like there should at least be a message that you aren't seeing all results (maybe with a button that lets you load the whole object).  Or maybe once a filter starts we can eagerly load the whole thing?

Comment 19

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=618b51355eb9

(In reply to Brian Grinstead [:bgrins] from comment #18)
> (In reply to Alexandre Poirot [:ochameau] from comment #17)
> > Also, the filter issue is really painful, but I'm wondering if I should
> > block on it, it sounds like a quite significant work as well.
> 
> It seems like there should at least be a message that you aren't seeing all
> results (maybe with a button that lets you load the whole object).  Or maybe
> once a filter starts we can eagerly load the whole thing?

I'm more tempted to do the first, but it would introduce a string...
I'll look at how difficult it would be to implement a smart search involving server feature once this first patch is ready (waiting for green try now)!

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c69a5b1905
It looks like it introduces a crash in browser_console_variables_view_while_debugging_and_inspecting.js
  Assertion failure: mIsMainThread != MAIN_THREAD || !ipc::ParentProcessIsBlocked()

Comment 21

[Victor Porof [:vporof][:vp]]

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> (In reply to Victor Porof [:vporof][:vp] from comment #11)
> > (In reply to Victor Porof [:vporof][:vp] from comment #10)
> > > 
> > > This is a much better place to do it though
> > 
> > And by "this" I mean the controller.
> 
> Should I just drop the similar code from VariableView.jsm? 
> As I highlighted during last meeting, we have a bunch of code to drop if we
> switch to pure-e10s and ignore local codepath with direct access to object.

Yes. All this stuff should be done in a single place only.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd8f7d3754a
Actually, I messed up with skip-if this test was already blacklisted on e10s...
This patch is ready for review, I would just like yo followup with VariableView.jsm cleanup
(to remove the previous non-e10s feature) and with another patch to implement filter
via a new actor request.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Remove old non-e10s code to paginate properties - v1

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: Filter properties remotely - v1

Here is a sketch of how we could filter properties with a PropertyIterator, remotely.

That's the same thing... That's something we used to do on the client side in VariableView.jsm
but now we have to move it to VariableViewController as it involves the actor.
I had to hack around to save the objectActor instance in order to make it available later in VariableViewController.performSearch.
I also consider that there is only one Scope() and empty it during each search.

Feedback welcomed again!

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8605250 [details] [diff] [review]
Remove old non-e10s code to paginate properties - v1

Note that the previous patch already disabled this code as I increased APPEND_PAGE_SIDE_DEFAULT to 2000.
This will prevent paging to work against old devices, but it will be very slow(crashy) anyway if you try to inspect big arrays...

Comment 26

[Alexandre Poirot [:ochameau]]

Jeff, here is a macos try if you want to regive it a try. I should have addressed all your comments with all these patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5547a63307df

Comment 27

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

Comment on attachment 8604770 [details] [diff] [review]
patch v5

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

I have a few comments and suggestions, but this looks great. I saw that you posted another patch though, so I might as well take another look at the combined patch.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +11,5 @@
>  const DBG_STRINGS_URI = "chrome://browser/locale/devtools/debugger.properties";
>  const LAZY_EMPTY_DELAY = 150; // ms
>  const LAZY_EXPAND_DELAY = 50; // ms
>  const SCROLL_PAGE_SIZE_DEFAULT = 0;
> +const APPEND_PAGE_SIZE_DEFAULT = 2000;

Does this have to be the same as MAX_PROPERTY_ITEMS?

@@ +1535,5 @@
>      }
>      this._isExpanded = true;
>  
>      if (this.onexpand) {
> +      return this.onexpand(this);

Why this change? It doesn't look like the return value of this method is used anywhere. If it is, then please add a comment indicating its potential values.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +169,5 @@
> +   *        The Scope where the properties will be placed into.
> +   * @param object aGrip
> +   *        The property iterator grip.
> +   */
> +  _populatePropertySlices: function(aTarget, aGrip, aIterator) {

aIterator is missing from the method comments.

@@ +180,5 @@
> +
> +    let promises = [];
> +    for(let i = 0; i < 4; i++) {
> +      let start = aGrip.start + i * items;
> +      let count = Math.min(items, aGrip.count - (i * items));

I think this would be slightly faster (no method call overhead, no trip from JS -> C++ -> JS):

let count = i != 3 ? items : aGrip.count - i * items;

I'm not sure how you feel about its clarity though, so up to you.

@@ +236,5 @@
> +  },
> +
> +  /**
> +   * Adds the properties for a Variable in the view using new feature from FF40+,
> +   * where we can iterate over properties by slices.

Nit: "using a new feature in FF40+ that allows iteration over properties in slices."

@@ +250,5 @@
> +    let deferred = promise.defer();
> +    let objectClient = this._getObjectClient(aGrip);
> +    let isArray = aGrip.preview && aGrip.preview.kind === "ArrayLike";
> +    if (isArray) {
> +      // First enumerate array items. e.g. properties from `0` to `array.length`.

Typo: comma instead of period before "e.g."

@@ +264,5 @@
> +          count: iterator.count
> +        };
> +        this._populatePropertySlices(aTarget, sliceGrip, iterator)
> +            .then(() => {
> +          // Then enumerate other properties. Most likely named properties like length, buffer, ...

Nit: "Then enumerate the rest of the properties, like length, buffer, etc."

@@ +265,5 @@
> +        };
> +        this._populatePropertySlices(aTarget, sliceGrip, iterator)
> +            .then(() => {
> +          // Then enumerate other properties. Most likely named properties like length, buffer, ...
> +          options = {

This is a different scope, so better use |let| here.

@@ +312,5 @@
> +      // Add the variable's __proto__.
> +      if (prototype && prototype.type != "null") {
> +        let proto = aTarget.addItem("__proto__", { value: prototype });
> +        this.addExpander(proto, prototype);
> +      }

This part is shared with _populateProperties, why not extract it into a function? It may be that this is all that _populateObjectPrototype should contain and the rest should be inlined in _populateFromObject.

::: toolkit/devtools/client/dbg-client.jsm
@@ +2230,5 @@
> +   * Request a PropertyIteratorClient instance to ease listing
> +   * properties for this object.
> +   *
> +   * @param options Object
> +   *        A dictionnary object with various boolean attributes:

Typo: dictionary

@@ +2233,5 @@
> +   * @param options Object
> +   *        A dictionnary object with various boolean attributes:
> +   *        - ignoreSafeGetters Boolean
> +   *          If true, do not iterate over safe getters.
> +   *        - onlyArrayItems Boolean

onlyIndexedProperties would be more accurate.

@@ +2236,5 @@
> +   *          If true, do not iterate over safe getters.
> +   *        - onlyArrayItems Boolean
> +   *          If true, only iterate over Array items.
> +   *          e.g. properties names between `0` and `object.length`.
> +   *        - onlyNonArrayItems Boolean

onlyNonIndexedProperties would be the complement to the above, but there is a problem when options can be combined in nonsensical ways. What would the proper response be if both of these options were true?

For that reason, I think a better approach would be to use the opposite flags: ignoreIndexedProperties and ignoreNonIndexedProperties. They fit nicely with ignoreSafeGetters, can be sensibly combined (both true: empty set, both false: all the properties) and if so desired we could use default parameter values in various methods that simplify the common case.

@@ +2315,5 @@
>  /**
> + * A PropertyIteratorClient provides a way to access to property names and
> + * values of an object efficiently, slice by slice.
> + * Note that the properties are sorted while constructing the
> + * PropertyIteratorActor, when calling TabActor.enumProperties.

I think it would be clearer to say "the properties are sorted in the backend" here, even if you want to include the technical details afterwards. Often people who work at the frontend are oblivious to how the backend works and this information would be confusing to them.

@@ +2338,5 @@
> +   */
> +  get count() { return this._grip.count; },
> +
> +  /**
> +   * Get one or many property name(s).

Nit: "Get one or more property names that correspond to the positions in the indexes parameter."

@@ +2356,5 @@
> +   *
> +   * @param start Number
> +   *        The index of the first property to fetch.
> +   * @param count Number
> +   *        The number of property to fetch.

Typo: properties

::: toolkit/devtools/server/actors/root.js
@@ +170,5 @@
>        return DebuggerServer.allowChromeProcess;
>      },
>      // Whether or not `getProfile()` supports specifying a `startTime`
>      // and `endTime` to filter out samples. Fx40+
> +    profilerDataFilterable: true,

Nit: this is redundant.

::: toolkit/devtools/server/actors/script.js
@@ +3291,5 @@
>  };
>  
> +
> +/**
> + * Creates an actor to iterate over an object property names and values.

"over an object's property names and values" or "over the property names and values of an object".

@@ +3296,5 @@
> + *
> + * @param aObjectActor ObjectActor
> + *        The object actor.
> + * @param aOptions Object
> + *        A dictionnary object with various boolean attributes:

Typo: dictionary

@@ +3318,5 @@
> +  let ownProperties = Object.create(null);
> +  let names = [];
> +  try {
> +    names = this.objectActor.obj.getOwnPropertyNames();
> +  } catch (ex) {}

What is the try block here for? The MouseEvent or the internal functions bus? Can we have a comment here if this isn't going to be fixed before the patch lands?

@@ +3338,5 @@
> +  if (aOptions.onlyArrayItems || aOptions.onlyNonArrayItems) {
> +    let length = DevToolsUtils.getProperty(this.objectActor.obj, "length");
> +    if (typeof(length) !== "number") {
> +      // Pseudo arrays are flagged as ArrayLike if they have
> +      // subsequent integer properties without having any length attribute.

Nit: "indexed properties" not "integer properties"

@@ +3371,5 @@
> +    let desc = this.objectActor._propertyDescriptor(name);
> +    if (!desc) {
> +      desc = safeGetterValues[name];
> +    }
> +    if (name in safeGetterValues) {

Why isn't this if block combined with the previous one?

@@ +3479,5 @@
>  
> +      // FF40+: Allow to know how many properties an object has
> +      // to lazily display them when there is a bunch.
> +      // Throws on some MouseEvent object in tests.
> +      try {

So is the try block for the MouseEvent bug or for the internal function assertion? Can we ping jimb or DOM folks about the former?

Comment 28

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

Comment on attachment 8605250 [details] [diff] [review]
Remove old non-e10s code to paginate properties - v1

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

Looks good, but I'll take a closer look when you combine it with the other patch.

Comment 29

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

Comment on attachment 8605251 [details] [diff] [review]
Filter properties remotely - v1

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

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +631,5 @@
> +    }
> +    this.view.empty();
> +    let scope = this.view.addScope("search");
> +    scope.expanded = true; // Expand the scope by default.
> +    scope.locked = true; // Prevent collpasing the scope.

Ideally this would just return the results to VariablesView.jsm, which would deal with displaying the results.

Comment 30

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #27)
> Comment on attachment 8604770 [details] [diff] [review]
>
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +11,5 @@
> >  const DBG_STRINGS_URI = "chrome://browser/locale/devtools/debugger.properties";
> >  const LAZY_EMPTY_DELAY = 150; // ms
> >  const LAZY_EXPAND_DELAY = 50; // ms
> >  const SCROLL_PAGE_SIZE_DEFAULT = 0;
> > +const APPEND_PAGE_SIZE_DEFAULT = 2000;
> 
> Does this have to be the same as MAX_PROPERTY_ITEMS?

It just has to be greater, otherwise we get both spliting code in action and that looks really weird!
That simply allows to disable the splitting code from VariableView and let VariableViewController one take the lead. But we will fallback to VariableView one if the actor doesn't support new features.

> 
> @@ +1535,5 @@
> >      }
> >      this._isExpanded = true;
> >  
> >      if (this.onexpand) {
> > +      return this.onexpand(this);
> 
> Why this change?

Mostly for tests, in order to know when the expand is done.

> It doesn't look like the return value of this method is
> used anywhere. If it is, then please add a comment indicating its potential
> values.

I'll!

> @@ +312,5 @@
> > +      // Add the variable's __proto__.
> > +      if (prototype && prototype.type != "null") {
> > +        let proto = aTarget.addItem("__proto__", { value: prototype });
> > +        this.addExpander(proto, prototype);
> > +      }
> 
> This part is shared with _populateProperties, why not extract it into a
> function? It may be that this is all that _populateObjectPrototype should
> contain and the rest should be inlined in _populateFromObject.

done.

> @@ +2236,5 @@
> > +   *          If true, do not iterate over safe getters.
> > +   *        - onlyArrayItems Boolean
> > +   *          If true, only iterate over Array items.
> > +   *          e.g. properties names between `0` and `object.length`.
> > +   *        - onlyNonArrayItems Boolean
> 
> onlyNonIndexedProperties would be the complement to the above, but there is
> a problem when options can be combined in nonsensical ways. What would the
> proper response be if both of these options were true?
> 
> For that reason, I think a better approach would be to use the opposite
> flags: ignoreIndexedProperties and ignoreNonIndexedProperties. They fit
> nicely with ignoreSafeGetters, can be sensibly combined (both true: empty
> set, both false: all the properties) and if so desired we could use default
> parameter values in various methods that simplify the common case.

Ok, WFM.

> 
> @@ +2315,5 @@
> >  /**
> > + * A PropertyIteratorClient provides a way to access to property names and
> > + * values of an object efficiently, slice by slice.
> > + * Note that the properties are sorted while constructing the
> > + * PropertyIteratorActor, when calling TabActor.enumProperties.
> 
> I think it would be clearer to say "the properties are sorted in the
> backend" here, even if you want to include the technical details afterwards.
> Often people who work at the frontend are oblivious to how the backend works
> and this information would be confusing to them.

Actually I didn't meant to really mention the backend, but emphasize that you can control sorting in the enumProperties call. So ObjectClient.enumProperties.
I've updated the comment to:
 * Note that the properties can be sorted in the backend,
 * this is controled while creating the PropertyIteratorClient
 * from ObjectClient.enumProperties.

> ::: toolkit/devtools/server/actors/root.js
> @@ +170,5 @@
> >        return DebuggerServer.allowChromeProcess;
> >      },
> >      // Whether or not `getProfile()` supports specifying a `startTime`
> >      // and `endTime` to filter out samples. Fx40+
> > +    profilerDataFilterable: true,
> 
> Nit: this is redundant.

Hum, it looks like git is playing with me here!

> @@ +3318,5 @@
> > +  let ownProperties = Object.create(null);
> > +  let names = [];
> > +  try {
> > +    names = this.objectActor.obj.getOwnPropertyNames();
> > +  } catch (ex) {}
> 
> What is the try block here for? The MouseEvent or the internal functions
> bus? Can we have a comment here if this isn't going to be fixed before the
> patch lands?

I think I just used the try/catch we are already using everywhere we are using getOwnPropertyNames.

It's not going to be fixed, it's just that getOwnPropertyNames is busted in various cases.
I've identified at least:
- the mouseevent exception: I've not looked into it, but it was throwing when creating the actor's grip, I'll see if that throws here at well.
- internal function crash: Reported, bug 1163520, but very limited help from jimb.

> @@ +3371,5 @@
> > +    let desc = this.objectActor._propertyDescriptor(name);
> > +    if (!desc) {
> > +      desc = safeGetterValues[name];
> > +    }
> > +    if (name in safeGetterValues) {
> 
> Why isn't this if block combined with the previous one?

I could do if (!desc) ... else if (name in safeGetterValues) ...
but I don't see how to make it much simplier.
If the is no regular descriptor, we fallback to safeGetterValues descriptor.
Otherwise, if there is a regular descriptor, we augment it with getterValue/getterPrototypeLevel.
This replicates some existing client code over here:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesViewController.jsm#190

> 
> @@ +3479,5 @@
> >  
> > +      // FF40+: Allow to know how many properties an object has
> > +      // to lazily display them when there is a bunch.
> > +      // Throws on some MouseEvent object in tests.
> > +      try {
> 
> So is the try block for the MouseEvent bug or for the internal function
> assertion? Can we ping jimb or DOM folks about the former?

The try/catch is only for MouseEvent, whereas the if(this.obj.class != "Function") is for the internal function crash.

Comment 31

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #29)
> Comment on attachment 8605251 [details] [diff] [review]
> Filter properties remotely - v1
> 
> Review of attachment 8605251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/VariablesViewController.jsm
> @@ +631,5 @@
> > +    }
> > +    this.view.empty();
> > +    let scope = this.view.addScope("search");
> > +    scope.expanded = true; // Expand the scope by default.
> > +    scope.locked = true; // Prevent collpasing the scope.
> 
> Ideally this would just return the results to VariablesView.jsm, which would
> deal with displaying the results.

I tried to return the Scope object created here, and call empty() from VariableView:_doSearch, but the tricky part is that we have to call empty() before addScope() whereas we know if we should call empty only once we receive the result of controller.performSearch() :/
Otherwise controller.performSearch can't return something else than a Scope,
only the controller knows how to populate the Scope object from actor's data.
What I could do is something a bite more complex like this:
VariablesView._doSearch = () => {
  if (controller.supportsSearch()) {
    this.empty();
    let scope = this.addScope("Search");
    scope.xxx = true;
    ...
    controller.performSearch(scope, aToken);
  }
}
VariablesViewController.supportsSearch = () => {
  // FF40+ starts exposing ownPropertyLength on object actor's grip
  // as well as enumProperty which allows to query a subset of properties.
  return this.objectActor && "ownPropertyLength" in this.objectActor;
}
VariablesViewController.performSearch = (scope, token) => {
  this._populateFromObjectWithIterator(scope, this.objectActor, token);
}
Do you think that's any better?

Comment 32

[Alexandre Poirot [:ochameau]]

Attachment: interdiff v5 -> v6

interdiff on top of attachment 8604770 [details] [diff] [review]

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: interdiff v5 -> v6

Comment 34

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #30)
> (In reply to Panos Astithas [:past] from comment #27)
> > Comment on attachment 8604770 [details] [diff] [review]
>
> > @@ +2236,5 @@
> > > +   *          If true, do not iterate over safe getters.
> > > +   *        - onlyArrayItems Boolean
> > > +   *          If true, only iterate over Array items.
> > > +   *          e.g. properties names between `0` and `object.length`.
> > > +   *        - onlyNonArrayItems Boolean
> > 
> > onlyNonIndexedProperties would be the complement to the above, but there is
> > a problem when options can be combined in nonsensical ways. What would the
> > proper response be if both of these options were true?
> > 
> > For that reason, I think a better approach would be to use the opposite
> > flags: ignoreIndexedProperties and ignoreNonIndexedProperties. They fit
> > nicely with ignoreSafeGetters, can be sensibly combined (both true: empty
> > set, both false: all the properties) and if so desired we could use default
> > parameter values in various methods that simplify the common case.
> 
> Ok, WFM.

I did that in my last patch (v6) but I'm wondering. Wouldn't it be better to use a list of ignore strings:
  objectClient.enumProperties({ignore: ["safe-getters", "indexed-properties", "non-indexed-properties"]}, ...);
instead of pilling up boolean flags like that. I think I got suggested to use such string list instead of booleans in another actor.

Comment 35

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

See attachment 8607044 [details] [diff] [review] for interdiff.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c6f8b662624

Comment 36

[Alexandre Poirot [:ochameau]]

Attachment: Filter properties remotely - v2

rebased.

Comment 37

[Alexandre Poirot [:ochameau]]

Attachment: interdiff v6 -> v7

Fixed ignoreIndexedProperties behavior and race around __proto__ in test.

Comment 38

[Alexandre Poirot [:ochameau]]

Attachment: patch v7

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40d5bd57437b

Comment 39

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

(In reply to Alexandre Poirot [:ochameau] from comment #34)
> I did that in my last patch (v6) but I'm wondering. Wouldn't it be better to
> use a list of ignore strings:
>   objectClient.enumProperties({ignore: ["safe-getters",
> "indexed-properties", "non-indexed-properties"]}, ...);
> instead of pilling up boolean flags like that. I think I got suggested to
> use such string list instead of booleans in another actor.

I don't think I've ever seen this pattern used in our code. Do you have a reference for that discussion? Using an options object with boolean properties is quite common however, e.g. the traits object or the instances in the debugger client.

Comment 40

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

Comment on attachment 8607238 [details] [diff] [review]
patch v7

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1536,5 @@
>      this._isExpanded = true;
>  
>      if (this.onexpand) {
> +      // We return onexpand as it sometimes returns a promise
> +      // (up the the user of VariableView to do it)

Nit: the sentence in parens needs rewording.

::: toolkit/devtools/server/actors/root.js
@@ +170,5 @@
>        return DebuggerServer.allowChromeProcess;
>      },
>      // Whether or not `getProfile()` supports specifying a `startTime`
>      // and `endTime` to filter out samples. Fx40+
> +    profilerDataFilterable: true,

Haha, still here!

::: toolkit/devtools/server/actors/script.js
@@ +3359,5 @@
> +    }
> +
> +    if (aOptions.ignoreNonIndexedProperties) {
> +      names = names.filter(i => {
> +        return Number.isInteger(Number(i)) && i >= 0 && i < length;

Don't you implicitly expect an integer i in the second and third conditions? It would be better to be more explicit about it and at the same time be more consistent with the filter in the ignoreIndexedProperties case above, by using "i = parseFloat(i)" first.

@@ +3374,5 @@
> +    let desc = this.objectActor._propertyDescriptor(name);
> +    if (!desc) {
> +      desc = safeGetterValues[name];
> +    }
> +    else if (name in safeGetterValues) {

Can you also add the "Merge the safe getter values..." comment here as well?

Comment 41

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #39)
> I don't think I've ever seen this pattern used in our code. Do you have a
> reference for that discussion? Using an options object with boolean
> properties is quite common however, e.g. the traits object or the instances
> in the debugger client.

I can't remember exactly what/where I got this suggestion but can't find any such property in existing actors.
That must be something in WebIDE UI codebase then...

Comment 42

[Alexandre Poirot [:ochameau]]

Attachment: patch v8

Addressed review comments and merged with removal of non-e10s code.

Comment 43

[Alexandre Poirot [:ochameau]]

Attachment: Filter properties remotely - v3

Rebased.

Comment 44

[Alexandre Poirot [:ochameau]]

Comment on attachment 8610057 [details] [diff] [review]
Filter properties remotely - v3

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

Let's see the filtering now.

About your comment 29, see my reply in comment 31.

Comment 45

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

Comment on attachment 8610057 [details] [diff] [review]
Filter properties remotely - v3

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

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +633,5 @@
> +    }
> +    this.view.empty();
> +    let scope = this.view.addScope("search");
> +    scope.expanded = true; // Expand the scope by default.
> +    scope.locked = true; // Prevent collpasing the scope.

Typo: collapsing

::: toolkit/devtools/server/actors/script.js
@@ +3268,5 @@
>   *          If true, the iterator will sort the properties by name
>   *          before dispatching them.
> + *        - query String
> + *          If non-empty, will filter the properties by names containing
> + *          this query string.

A note that the matching is not case-sensitive is warranted here.

@@ +3329,5 @@
> +  if (aOptions.query) {
> +    let { query } = aOptions;
> +    query = query.toLowerCase();
> +    names = names.filter(name => {
> +      return name.indexOf(query) !== -1;

You could also use the shorter form "name.includes(query)" if you prefer.

Comment 46

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

(In reply to Alexandre Poirot [:ochameau] from comment #31)
> Do you think that's any better?

That's definitely better from a separation of concerns POV, but I just looked at the controller again and it seems that there are a lot of similar cases there already, so it's not going to make a huge difference one way or the other. Whatever you prefer.

Comment 47

[Alexandre Poirot [:ochameau]]

Attachment: patch v9

Merged the two patches and addressed last review comments.

(In reply to Panos Astithas [:past] from comment #46)
> (In reply to Alexandre Poirot [:ochameau] from comment #31)
> > Do you think that's any better?
> 
> That's definitely better from a separation of concerns POV,

I did what I explained in #31 then.

Comment 48

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7407608235

Comment 49

[Pulsebot]

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

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8653d81c62e4

Comment 51

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 33.0a1 (Build ID:20140610030202) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 43.0a1(Build ID:20150908030203)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

Verified as fixed with Firefox beta 41.0b8 (Build ID: 20150907144446)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

Verified as fixed with Firefox aurora 42.0a2 (Build ID:20150908004020)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

[testday-20150904]

Comment 52

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug in Nightly 33.0a1 (2014-06-10) by following comment 0's instructions on Linux x64

This Bug is now Verified Fixed on Latest Firefox Beta 41.0b9

Build ID: 20150910171927
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

As it is also verified on Windows (Comment 51), Marking it as verified!