Closed Bug 1480610 Opened Last year Closed Last year

Add fxmonitor system add-on stub to m-c build system

Categories

(Firefox :: Firefox Monitor, enhancement)

62 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 + fixed
firefox63 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
Adds the boilerplate for the extension.

mythmon: Check out privileged/FirefoxMonitor.jsm. The init function in that module is the entry point of the feature and has the pref code - let me know if it looks ok.

Thanks!
Attachment #8997967 - Flags: review?(mcooper)
Attachment #8997967 - Flags: review?(jhofmann)
Attached patch Patch (obsolete) — Splinter Review
Forgot to hg add a file. Please see the previous comment for details about the patch.
Attachment #8997967 - Attachment is obsolete: true
Attachment #8997967 - Flags: review?(mcooper)
Attachment #8997967 - Flags: review?(jhofmann)
Attachment #8997968 - Flags: review?(mcooper)
Attachment #8997968 - Flags: review?(jhofmann)
Comment on attachment 8997968 [details] [diff] [review]
Patch

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

The pref handling here is almost too little to be sure. It looks like it is going in the right direction, since you're using a pref observer. I think this will be fine for Normandy compatibility.
Attachment #8997968 - Flags: review?(mcooper) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
Fixed some minor style inconsistencies.
Attachment #8997968 - Attachment is obsolete: true
Attachment #8997968 - Flags: review?(jhofmann)
Attachment #8997972 - Flags: review?(jhofmann)
Comment on attachment 8997972 [details] [diff] [review]
Patch v1.1

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

This looks good overall but we should make sure we're not leaking pref observers (maybe this works for some reason because it's a webextension but we should still unregister explicitly).

::: browser/extensions/fxmonitor/privileged/FirefoxMonitor.jsm
@@ +6,5 @@
> +  extension: null,
> +  init(aExtension) {
> +    this.extension = aExtension;
> +
> +    Preferences.observe(this.kEnabledPref, () => {

This needs a Preferences.ignore equivalent.

https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/toolkit/modules/Preferences.jsm#281

Does this pass on try? :)
Attachment #8997972 - Flags: review?(jhofmann) → review-
Comment on attachment 8998205 [details] [diff] [review]
Patch v2

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

Thanks!
Attachment #8998205 - Flags: review?(jhofmann) → review+
Attached patch Patch v2 for beta (obsolete) — Splinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Not a regression. Feature is bug 1478521
[User impact if declined]: This is for a new feature, intended to be released in the September marketing season.
[Is this code covered by automated tests?]: It introduces boilerplate for a new system add-on; no functionality; try push links in comment 6.
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No, but QA will be testing this thoroughly as the feature evolves.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds boilerplate. No functionality. Introduces a pref (off by default) behind which future functionality additions will be added. 
[String changes made/needed]: None.
Attachment #8998225 - Flags: approval-mozilla-beta?
Attachment #8998205 - Flags: checkin+
Attachment #8998205 - Flags: checkin+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ad28f117cf
Implement Firefox Monitor add-on stub. r=johannh
Keywords: checkin-needed
Comment on attachment 8998225 [details] [diff] [review]
Patch v2 for beta

Clearing approval request while I figure out the issues that led to backout.
Flags: needinfo?(nhnt11)
Attachment #8998225 - Flags: approval-mozilla-beta?
Attached patch Patch v3 (obsolete) — Splinter Review
Stubs this patch even more, fixes eslint failures.
Attachment #8998205 - Attachment is obsolete: true
Attachment #8998507 - Flags: review?(jhofmann)
Attachment #8998225 - Attachment is obsolete: true
Attachment #8998507 - Flags: review?(jhofmann) → review+
Luca, as I mentioned on IRC, adding this stub system add-on is causing an asan failure in browser_ext_addon_debugging_netmonitor.js. You're the author of that file, so I was wondering if you could help me figure out why my patch is causing an asan failure there and what we could do to fix it.

Reference try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14fbd7a62db6cf7576741e2639f65e78edc6346&selectedJob=192800506

Log of the error: https://treeherder.mozilla.org/logviewer.html#?job_id=192800506&repo=try&lineNumber=1419

Thanks!
Flags: needinfo?(lgreco)
Wennie, is this something that still need to land in  62 beta for this feature to be rolled out during 62 release?
Flags: needinfo?(wleung)
As an effort to move this along, I've stripped the patch down even further to the bare minimum we need to uplift in order to be able to push updates to the add-on. I've pushed it to try to see if the asan failure goes away.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b4a2eb1a88f002bb954db98c3ff504738aa8e5
Johann, this strips down the patch to the bare minimum required to register the add-on. This eliminates the asan failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b4a2eb1a88f002bb954db98c3ff504738aa8e5&selectedJob=192828813.
Attachment #8998507 - Attachment is obsolete: true
Attachment #8998583 - Flags: review?(jhofmann)
Comment on attachment 8998583 [details] [diff] [review]
Ultra minimal patch

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

I also would have been fine with filing a bug and disabling the test for now :)
Attachment #8998583 - Flags: review?(jhofmann) → review+
This simply rebases browser/extensions/moz.build on top of the beta head. Carrying the r+.
Attachment #8998585 - Flags: review+
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e23fa2eb65d
Add minimal stub for fxmonitor add-on into build system. r=johannh
Keywords: checkin-needed
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Wennie, is this something that still need to land in  62 beta for this
> feature to be rolled out during 62 release?

Liz, not sure if you clarified this with Wennie already outside of Bugzilla, but yes, this patch is the minimum that needs to be uplifted to beta in order to enable shipping Firefox Monitor during 62. Please let me know if there's any other info I can provide. I'm taking the liberty to clear Wennie's needinfo? request, please re-request if necessary.
Flags: needinfo?(wleung)
OK, please request uplift once it lands. Thanks!
https://hg.mozilla.org/mozilla-central/rev/3e23fa2eb65d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8998585 [details] [diff] [review]
Ultra minimal patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression. Feature is bug 1478521
[User impact if declined]: This is for a new feature, intended to be released in the September marketing season.
[Is this code covered by automated tests?]: It introduces boilerplate for a new system add-on; no functionality.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No, but QA will be testing this thoroughly as the feature evolves.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds boilerplate. No functionality. Just an add-on manifest and the relevant moz.build changes.
[String changes made/needed]: None.
Attachment #8998585 - Flags: approval-mozilla-beta?
Comment on attachment 8998585 [details] [diff] [review]
Ultra minimal patch for beta

Let's uplift for beta 17 as it's planned for this feature in 62/63. 
As I understand it this will be off by default when we ship 62 but then will be rolled out after 62 release (so there is a bit of extra runway for testing).
Attachment #8998585 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1483430
(In reply to Nihanth Subramanya [:nhnt11] from comment #14)
> Luca, as I mentioned on IRC, adding this stub system add-on is causing an
> asan failure in browser_ext_addon_debugging_netmonitor.js. You're the author
> of that file, so I was wondering if you could help me figure out why my
> patch is causing an asan failure there and what we could do to fix it.

Hi Nihanth,
Sorry I forgot to reply to this needinfo, we met a similar issue in Bug 1449055 and
we figured out how to prevent those asan failures, the fix landed as part of Bug 1449055 in:

https://hg.mozilla.org/mozilla-central/rev/774f0b7aba00
Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.