871423 - Link to inline style sheet in inspector opens incorrect editor in Style Editor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Heather Arthur [:harth]]

1) Visit a site with multiple inline stylesheets, like http://google.com
2) Inspect an element with styles (like the google logo)
3) Click on the inline stylesheet link for one of the rules

The first inline stylesheet is always opened in the Style Editor, regardless of which stylesheet contained the rule. This is because the editor is looking for the style sheet with the same href, which is `undefined` for all inline stylesheets. It should probably be looking for the sheet with same index in the document's stylesheet list.

Comment 1

[Heather Arthur [:harth]]

Attachment: WIP, leaks memory running mochitest-devtools

Here's a WIP that adds a WeakMap of JS Object -> Actors to the TabActor, that both the inspector and style editor can use to ensure they're using the same actor for a stylesheet.

However, this leaks DOM windows when the style editor tests are run.

Comment 2

[Heather Arthur [:harth]]

Attachment: WIP, leaks memory running styleeditor tests

Revisiting this bug and admitting that I need help.

Apply this patch and the original bug is fixed (style inspector links will open the exact inline stylesheet they should). It does this by putting a WeakMap in the TabActor that both the style editor and inspector share to ensure there's one StyleSheetActor per stylesheet object.

It works but if you run the style editor tests there are memory leaks. Run browser/devtools/styleeditor/test/browser_styleeditor_private_perwindowpb.js to see this x4:

1085 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_private_perwindowpb.js | leaked until shutdown [nsGlobalWindow #244 outer  about:blank] - leaked until shutdown [nsGlobalWindow #244 outer  about:blank]

If I comment out the line where I add the StyleSheetActor to the TabActor's objectToActorMap, there is no leak. I clear the objectToActorMap on disconnect. I tried also deleting the stylesheet from the map on StyleSheetActor's destroy(), but its destroy method is never called. The StyleSheetFront's destroy() is always called, so I thought protocol.js would call the actor's destroy automatically, but is that something I manually have to call?

I'd like to either 1) know how the WeakMap is causing the memory leak, especially when it's cleared on disconnect or 2) Maybe another way of doing this altogether.

Any help is appreciated.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8467466 [details] [diff] [review]
WIP, leaks memory running styleeditor tests

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

Would be really nice to have bug 960675 right now...

Every object keeps its global alive, so it might not actually be the WeakMap that is leaking, but something else. I looked for closures that might accidentally be keeping stuff alive, but didn't see anything that jumped out.

What is the lifetime hierarchy after these changes? I'd expect that the stylesheet actors would be children of the tab actor now, but it looks like you didn't change anything. If I understand correctly, this means that it's a race between the inspector and style editor actors to create the stylesheet actor for a given stylesheet, at which point that new actor is now a child of which ever actor won that race. Now the other actor that lost the race is holding a reference to an actor that's lifetime is potentially shorter than its own! (If the actor that won the race is destroyed, its child actors are destroyed as well, leaving the actor that lost the race with a reference to a now-destroyed actor). I know in practice pretty much all of this stuff lives as long as the tab lives, but in places like tests, maybe that's not the case and maybe this is contributing to the leak in the tests. These problems go away if stylesheet actors are children of the tab actor.

One alternative to is to replace the WeakMap with a plain old Map and manually clear and reset it on the tab actor's "window-ready" event. In fact, you should probably destroy old stylesheet actors on that event anyways, or else you will keep alive old stylesheet actors from previous navigations until the tab actor is destroyed and therefore destroys all of its children as well. In fact, this might be related to the current leak as well.

(Aside: you should add "unified = 8" to the "[diff]" section of your .hgrc so its easier to see context in your patches (: )

::: toolkit/devtools/server/actors/webbrowser.js
@@ +694,4 @@
>    disconnect: function BTA_disconnect() {
>      this._detach();
>      this._extraActors = null;
> +    this.objectToActorMap.clear();

Maybe try setting to null in addition to clearing?

Comment 4

[Heather Arthur [:harth]]

Attachment: Reduced case, using Map() that doesn't leak

Thanks Nick. That was really helpful. I made the StyleSheetActors children of the TabActor instead, which makes more sense. It still leaked when running those same tests however. Then I changed the WeakMap to a Map, and no leaks! That doesn't make sense to me, but I'll take it.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yay? :-/

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, so I just chatted with Terrence in #jsapi. The WeakMap implementation is a little trickier than the Map implementation when it comes to working with the GC (as you might suspect). It has additional read barriers that plain old Map doesn't which can conservatively keep the weak map alive across incremental GCs, but will be resolved at the next full GC. To determine if this is in fact the issue, he suggests forcing a full, non-incremental GC at the end of the test and seeing if mochitest leak detection still complains. If that doesn't fix it, then it may indeed be a bug in the WeakMap implementation.

https://developer.mozilla.org/en-US/docs/Components.utils.forceGC

In short, it may not actually be a leak, but a bug in mochitest leak detection with regards to understanding incremental GC.

Comment 7

[Heather Arthur [:harth]]

Okay, I put a Components.utils.forceGC(); right before the test finished, and it still leaked those four windows.

I'll use Map() in the meantime. Thanks for checking it out.

Comment 8

[Heather Arthur [:harth]]

Attachment: Share style sheet actors between inspector and style editor

Alright, this patch uses a Map in the TabActor, which is cleared when the tab navigates. Adds some tests for the rule view and computed view to test that clicking an inline style sheet link opens the correct editor in the style editor.

Comment 9

[Heather Arthur [:harth]]

Attachment: Share style sheet actors between inspector and style editor

Adding to root actor too in case of browser toolbox.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8469493 [details] [diff] [review]
Share style sheet actors between inspector and style editor

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

Heather, is this going to break a pre-Firefox 29 StyleEditorFront?  Do we care if it does?

Ryan, can you please take a look at the changes to root.js and webbrowser.js?  There are very similar changes in both to support sharing StyleSheetActors across tools.  Is there a place to share this code so it only needed to be implemented once, or is this the best way to add support the Browser Toolbox?

::: toolkit/devtools/server/actors/root.js
@@ +410,5 @@
> +    }
> +    let actor = new StyleSheetActor(styleSheet, this);
> +    this._styleSheetActors.set(styleSheet, actor);
> +
> +    this._globalActorPool.addActor(actor);

I notice this was `this.manage(actor)` when it was in stylesheets.js.  I'm not sure sure of the difference, but I'm assuming they are doing approximately the same thing.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8469493 [details] [diff] [review]
Share style sheet actors between inspector and style editor

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

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8469493 [details] [diff] [review]
> Share style sheet actors between inspector and style editor
> 
> Review of attachment 8469493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heather, is this going to break a pre-Firefox 29 StyleEditorFront?  Do we
> care if it does?

I believe the important compatibility scenarios are:

1. Nightly desktop client MUST maintain existing compatibility with Gecko 28 and later servers 
  * This is needed for supported B2G simulators (1.3+)
2. Nightly desktop client SHOULD maintain existing compatibility back to release channel servers
  * This is mainly to simplify cross-platform use cases, i.e. desktop Nightly with release Fennec
  * Scenario 1 above currently implies greater compatibility than this one anyway

Certainly when a new feature needs a new actor method to function, it won't work with servers that don't support it.  But we should still ensure the client doesn't explode when using unrelated, existing features, at least until the above time windows have elapsed.

You should test this patch with the Firefox OS 1.3 simulator to make sure existing features still work there.

I have just added these notes to the wiki[1] for future reference.  Please let me know if it seems like I've missed something!

> Ryan, can you please take a look at the changes to root.js and
> webbrowser.js?  There are very similar changes in both to support sharing
> StyleSheetActors across tools.  Is there a place to share this code so it
> only needed to be implemented once, or is this the best way to add support
> the Browser Toolbox?

We don't have a great answer here currently.  It would be great to put things like this into a shared module that both the root and browser make use of, so we don't have to repeat it.  But, if that's too hard, it could be done in a follow up.

[1]: https://wiki.mozilla.org/DevTools/Backwards_Compatibility

::: toolkit/devtools/server/actors/root.js
@@ +10,5 @@
>  const Services = require("Services");
>  const { ActorPool, appendExtraActors, createExtraActors } = require("devtools/server/actors/common");
>  const { DebuggerServer } = require("devtools/server/main");
>  const { dumpProtocolSpec } = require("devtools/server/protocol");
> +const { StyleSheetActor } = require("devtools/server/actors/stylesheets");

I get the impression that this will fail in the case of workers, because the actor seems to import things a worker wouldn't have.

Make this a lazy require so it's not done until first use.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +10,5 @@
>  let Services = require("Services");
>  let { ActorPool, createExtraActors, appendExtraActors } = require("devtools/server/actors/common");
>  let { RootActor } = require("devtools/server/actors/root");
>  let { AddonThreadActor, ThreadActor } = require("devtools/server/actors/script");
> +let { StyleSheetActor } = require("devtools/server/actors/stylesheets");

Lazy require here too.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8469493 [details] [diff] [review]
Share style sheet actors between inspector and style editor

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

Clearing review as per Ryan's comments.  It would be great if there was a shared function that could handle shared actors across root and webbrowser, if that seems doable here - I'm sure we will want to share other actor types going forward.

Comment 13

[Heather Arthur [:harth]]

Attachment: Share StyleSheetActors, plus lazy-loaded module

Did the lazy-loading of stylesheets.js. Tested on Simulator 1.3+ and it works on all.

As for a sharing the code between the browser and root actor, these actors should definitely go into a pool and map specific for that tab or browser as we don't want to keep them around longer than we should. Brian and I were talking about one possible solution to this which is to have something like:

function ActorRepository(actorMap, actorPool, parentActor) {
  this.actorMap = actorMap;
  this.actorPool = actorPool;
  this.parentActor = parentActor;
}

ActorRepository.prototype = {
  createStyleSheetActor: function(styleSheet) {
    if (this.actorMap.has(styleSheet)) {
      return this.actorMap.get(styleSheet);
    }
    let actor = new StyleSheetActor(styleSheet, this.parentActor);
    this.actorMap.set(styleSheet, actor);

    this.actorPool.addActor(actor);

    return actor;
  }
}

Then there could be one actor repository per tab actor and root actor.

Seems convoluted, but best I could come up with right now. Maybe something like that if we end up sharing more actors.

Comment 14

[Heather Arthur [:harth]]

Attachment: Share StyleSheetActors, plus lazy-loaded module

Forgot an import in root actor. Here's the final patch.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Heather Arthur  [:harth] from comment #13)
> Created attachment 8475498 [details] [diff] [review]
> Share StyleSheetActors, plus lazy-loaded module
> 
> Did the lazy-loading of stylesheets.js. Tested on Simulator 1.3+ and it
> works on all.
> 
> As for a sharing the code between the browser and root actor, these actors
> should definitely go into a pool and map specific for that tab or browser as
> we don't want to keep them around longer than we should. Brian and I were
> talking about one possible solution to this which is to have something like:
> 
> function ActorRepository(actorMap, actorPool, parentActor) {
>   this.actorMap = actorMap;
>   this.actorPool = actorPool;
>   this.parentActor = parentActor;
> }
> 
> ActorRepository.prototype = {
>   createStyleSheetActor: function(styleSheet) {
>     if (this.actorMap.has(styleSheet)) {
>       return this.actorMap.get(styleSheet);
>     }
>     let actor = new StyleSheetActor(styleSheet, this.parentActor);
>     this.actorMap.set(styleSheet, actor);
> 
>     this.actorPool.addActor(actor);
> 
>     return actor;
>   }
> }
> 
> Then there could be one actor repository per tab actor and root actor.
> 
> Seems convoluted, but best I could come up with right now. Maybe something
> like that if we end up sharing more actors.

We also just discussed adding a createStyleSheetActor function onto the ActorPool object (http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/common.js#87).

Then webbrowser.js would do:

createStyleSheetActor: function BTA_createStyleSheetActor(styleSheet) {
  return this._tabPool.createStyleSheetActor(styleSheet);
}

And root.js would do:

createStyleSheetActor: function(styleSheet) {
  return this._globalActorPool.createStyleSheetActor(styleSheet);
}

As long as we added a destroy() function to the ActorPool to cleanup and made sure that it was called at the correct time, I believe that this could work

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8475519 [details] [diff] [review]
Share StyleSheetActors, plus lazy-loaded module

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

I'd like to see some solution for sharing the createStyleSheetActor functionality, either as described in Comment 13 or Comment 15.  But this could be done in a follow up bug.  But given that backwards compat is supported, the code changes look good.

::: browser/devtools/styleinspector/test/browser_computedview_style-editor-link.js
@@ +130,5 @@
>    info("Validating style editor stylesheet");
>    let sheet = content.document.styleSheets[expectedSheetIndex];
>    is(editor.styleSheet.href, sheet.href, "loaded stylesheet matches document stylesheet");
>  }
> +

extra line

::: browser/devtools/styleinspector/test/browser_ruleview_style-editor-link.js
@@ +149,5 @@
>      "rule view stylesheet tooltip text matches the full URI path");
>  }
> +
> +function clickLinkByIndex(view, index) {
> +  let link =  getRuleViewLinkByIndex(view, index);

extra whitespace

::: toolkit/devtools/server/actors/root.js
@@ +407,5 @@
> +   * @return StyleSheetActor actor
> +   *         The actor for this style sheet.
> +   *
> +   */
> +  createStyleSheetActor: function BTA_createStyleSheetActor(styleSheet) {

Don't need the BTA_stuff in this file

Comment 17

[Heather Arthur [:harth]]

Attachment: Updated to comments

Try:
https://tbpl.mozilla.org/?tree=Try&rev=6c7beb638d26

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 19

[Heather Arthur [:harth]]

Attachment: Rebased for checkin

Small fix, sorry about that.

Comment 20

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/495a024e0c4d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/fx-team/rev/0cf8cdb30bb0

https://tbpl.mozilla.org/php/getParsedLog.php?id=46517960&tree=Fx-Team

Comment 22

[Heather Arthur [:harth]]

Attachment: + fix for xpcshell failure

This should fix it.

https://tbpl.mozilla.org/?tree=Try&rev=df4cb6dae0a2

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/95584aa8a5a9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95584aa8a5a9