964545 - Add-on SDK Page-mods should be listed in the website debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366487 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5

Irakli can I get your feedback on the sdk changes here please?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8366487 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5

As I suggested in the pull, I think it's better to use weak refs then relay on observer notifications to handle cleanup work, partially because those observer
notifications will likely change with E10S.

Either way I'm down with this.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8366487 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5

There is a patch file here https://github.com/mozilla/gecko-dev/pull/5.patch if it looks good.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/7e243174acf5

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: 964545p2.diff

This should fix the broken tests.

Comment 7

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> https://hg.mozilla.org/integration/fx-team/rev/7e243174acf5

Reverted in https://hg.mozilla.org/integration/fx-team/rev/45a117e6d254 for the test failures until the fix gets review.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: 964545p3.diff

With this path on top of the others the tests seem to pass https://tbpl.mozilla.org/?tree=Try&rev=ac1e629a3db6

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8367713 [details] [diff] [review]
964545p2.diff

>diff --git a/toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js
>index b28b050..65a405d 100644
>--- a/toolkit/devtools/server/actors/script.js
>+++ b/toolkit/devtools/server/actors/script.js
>@@ -5,13 +5,6 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> "use strict";
> 
>-//const { Cu } = require('chrome');
>-const { getInnerId } = require('sdk/window/utils');
>-
>-const { gDevToolsExtensions: {
>-  getContentGlobals
>-} } = Cu.import("resource://gre/modules/devtools/DevToolsExtensions.jsm", {});
>-
> let TYPED_ARRAY_CLASSES = ["Uint8Array", "Uint8ClampedArray", "Uint16Array",
>       "Uint32Array", "Int8Array", "Int16Array", "Int32Array", "Float32Array",
>       "Float64Array"];
>@@ -627,8 +620,18 @@ ThreadActor.prototype = {
>    */
>   globalManager: {
>     findGlobals: function () {
>+      const { getInnerId } = require('sdk/window/utils');
>+      const { gDevToolsExtensions: {
>+        getContentGlobals
>+      } } = Cu.import("resource://gre/modules/devtools/DevToolsExtensions.jsm", {});
>+
>       this.globalDebugObject = this._addDebuggees(this.global);
>-      getContentGlobals({ 'inner-window-id': getInnerId(this.global) }).forEach(this.addDebuggee.bind(this));
>+      // global may not be a window
>+      try {
>+        let id = getInnerId(this.global);
>+        getContentGlobals({ 'inner-window-id': id }).forEach(this.addDebuggee.bind(this));
>+      }
>+      catch(e) {}
>     },
> 
>     /**
>@@ -639,16 +642,23 @@ ThreadActor.prototype = {
>      *        The new global object that was created.
>      */
>     onNewGlobal: function (aGlobal) {
>+      const { getInnerId } = require('sdk/window/utils');
>+
>       let metadata = {};
>       try {
>         metadata = Cu.getSandboxMetadata(aGlobal.unsafeDereference());
>       }
>       catch (e) {}
> 
>+      let id;
>+      try {
>+        id = getInnerId(this.global);
>+      } catch(e) {}
>+
>       // Content debugging only cares about new globals in the contant window,
>       // like iframe children.
>       if ((metadata['inner-window-id'] &&
>-          metadata['inner-window-id'] == getInnerId(this.global)) ||
>+          metadata['inner-window-id'] == id) ||
>           (aGlobal.hostAnnotations &&
>           aGlobal.hostAnnotations.type == "document" &&
>           aGlobal.hostAnnotations.element === this.global)) {

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

The whole diff https://github.com/erikvold/gecko-dev/compare/964545v3

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #13)
> The whole diff https://github.com/erikvold/gecko-dev/compare/964545v3

Trying this here https://tbpl.mozilla.org/?tree=Try&rev=0e7a2ccdc2ea

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/72fdce22d68f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc crashes.
https://hg.mozilla.org/integration/fx-team/rev/21597c8c3d3c

https://tbpl.mozilla.org/php/getParsedLog.php?id=34077416&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34077136&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34077901&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34077717&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34078127&tree=Fx-Team

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

Ugh I should've noticed those browser chrome debug fails.. I'm not sure what is wrong there, obviously some change in the script.js file is causing this test to error, but I'm not sure how yet.. anyhow with this latest commit the inner-id code is not checked first but second, so that it does not run in many cases.  I'm just pushing that to try now to see if it helps, I'm not sure why it would though.

https://github.com/erikvold/gecko-dev/compare/964545v5
https://tbpl.mozilla.org/?tree=Try&rev=890273e75756

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

So when I comment out the line that uses the `Cu.getSandboxMetadata(aGlobal.unsafeDereference())` call all the tests pass except the added jetpack tests which would be expected to fail https://tbpl.mozilla.org/?tree=Try&rev=80cc23a11eec

https://github.com/erikvold/gecko-dev/compare/964545v8#diff-58d9dfd9ca670d23e714c743a045d90eR658

So I figure there is something wrong with `Cu.getSandboxMetadata(aGlobal.unsafeDereference())` and these bugs could be related:

https://bugzilla.mozilla.org/show_bug.cgi?id=926311
https://bugzilla.mozilla.org/show_bug.cgi?id=947792

Gabor do you think you could take a look at this for me please?

Comment 19

[Gabor Krizsanits (INACTIVE)]

Huh... I have very little context here but let me try. So we're crashing in nsWindowSH::OuterObject because the outer window's mJSObject is null, meaning probably that we are too early, and the outer window is not yet associated with the new global. unsafeDereference wraps the window, which comes with an outerization, that's how we end up here by the way. So it seems like calling unsafeDereference on windows in this onNewGlobal hook is bad, which I think is a problem, so I flag Bobby to help me out here if we need to address this issue.

My two cents for quick fixing your patch is to check somehow if the aGlobal is a window or a sandbox, and call unsafeDereference on it only if it's a sandbox (we don't care about window globals here if I get it right) then it should just work.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)

I think I can fix this, and filed bug 969156. Let me take a quick crack at it.

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

Trying https://github.com/erikvold/gecko-dev/compare/964545v6.patch at https://tbpl.mozilla.org/?tree=Try&rev=e20a3905fe85

Comment 22

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #21)
> Trying https://github.com/erikvold/gecko-dev/compare/964545v6.patch at
> https://tbpl.mozilla.org/?tree=Try&rev=e20a3905fe85

Trying this patch again

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

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/81c63efd1278

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/81c63efd1278

Comment 25

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/2bcae05879c9f5efeab9e29c9a8e7fb70b11b67c
Bug 964545 Add-on SDK page-mods are now debuggable r=dcamp

Comment 26

[Petruta Horea [:phorea], Desktop QA]

Is there a way this issue can be verified by manual QA? Thanks

Comment 27

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #26)
> Is there a way this issue can be verified by manual QA? Thanks

Install an add-on with a page-mod, open a tab that the page-mod should run on, open the website debugger, and you should see the page-mod listed.

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Thanks, Erik!

https://addons.mozilla.org/en-US/firefox/addon/geo-devtool/?src=userprofile
I only found this add-on but I don't manage to use it with new and classic google maps or other geolocation websites. I cc'ed Louis-Rémi maybe he can help.

I also tried to build an add-on but unfortunately without success.

Is anyone aware of any page-mod add-ons available on AMO? Thank you!

Comment 29

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> Thanks, Erik!
> 
> https://addons.mozilla.org/en-US/firefox/addon/geo-devtool/?src=userprofile
> I only found this add-on but I don't manage to use it with new and classic
> google maps or other geolocation websites. I cc'ed Louis-Rémi maybe he can
> help.
> 
> I also tried to build an add-on but unfortunately without success.
> 
> Is anyone aware of any page-mod add-ons available on AMO? Thank you!

Try the one below on google.com

https://addons.mozilla.org/en-US/firefox/addon/no-google-autocomplete/

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Erik, I can still see the Google autocomplete with the add-on enabled.
Also it doesn't seem to be a difference in the debugger of the google.com page between the builds with the add-on and the ones without it. Any thoughts on this?

Comment 31

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #30)
> Erik, I can still see the Google autocomplete with the add-on enabled.

Yes the add-on is a wip, it shouldn't matter for your test here though.

> Also it doesn't seem to be a difference in the debugger of the google.com
> page between the builds with the add-on and the ones without it. Any
> thoughts on this?

When I install the add-on I see it's page-mod in the debugger, so I'm not sure why you see something different.

Comment 32

[Petruta Horea [:phorea], Desktop QA]

Attachment: NightlyVSBeta.png

I can see the script in the debugger of the google.com page on Nightly 32.0a1 2014-05-29 under Win 7 64-bit, but it doesn't appear on Firefox 30 beta 9 and Aurora 31.0a2 2014-06-01.

This is strange because the fix reached central on 30th version.
Should the Target Milestone be set to Firefox 32?

Comment 33

[Erik Vold [:erikvold] (please needinfo? me)]

I think it's better to ask someone on devtools what is wrong here, they've done so much work on the debugger I haven't been able to keep track of it.