Closed Bug 964545 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: evold, Assigned: evold)

References

Details

Attachments

(4 files)

No description provided.
Assignee: nobody → evold
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?
Attachment #8366487 - Attachment description: https://github.com/mozilla/gecko-dev/pull/5 → Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5
Attachment #8366487 - Flags: feedback?(rFobic)
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.
Attachment #8366487 - Flags: feedback?(rFobic) → feedback+
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.
Attachment #8366487 - Flags: review?(dcamp)
Attachment #8366487 - Flags: review?(dcamp) → review+
Attached patch 964545p2.diffSplinter Review
This should fix the broken tests.
Attachment #8367713 - Attachment description: Fixing broken tests → Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5
Attachment #8367713 - Flags: review?(dcamp)
Attachment #8367713 - Attachment description: Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/5 → 964545p2.diff
(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.
Attachment #8367713 - Flags: review?(dcamp) → review+
Attached patch 964545p3.diffSplinter Review
With this path on top of the others the tests seem to pass https://tbpl.mozilla.org/?tree=Try&rev=ac1e629a3db6
Attachment #8369155 - Flags: review?(dcamp)
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)) {
(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
Attachment #8369155 - Flags: review?(dcamp) → review+
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
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?
Flags: needinfo?(gkrizsanits)
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.
Flags: needinfo?(gkrizsanits) → needinfo?(bobbyholley)
Depends on: 969156
(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.
Flags: needinfo?(bobbyholley)
Status: NEW → ASSIGNED
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/81c63efd1278
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Depends on: 898559
Depends on: 984929
Keywords: verifyme
Is there a way this issue can be verified by manual QA? Thanks
Flags: needinfo?(evold)
(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.
Flags: needinfo?(evold)
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!
(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/
Flags: needinfo?(evold)
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?
(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.
Attached image 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?
Flags: needinfo?(public)
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.
Flags: needinfo?(evold)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.