Remove the old Report Site Issue fallback and its addon, and have Report Broken Site perform its function
Categories
(Firefox :: General, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox138 | --- | fixed |
People
(Reporter: twisniewski, Assigned: twisniewski)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
Attachments
(1 file)
Historically, the Report Site Issue menu option was used to report broken sites in Firefox, which opens a tab to webcompat.com and sends it the URL and other data to pre-populate the form there. We've since replaced Report Site Issue with Report Broken Site, but since that relies on the telemetry engine to store the reports, we've retained Report Site Issue as a fallback when telemetry is disabled.
However, we can remove the Report Site Issue menu option entirely and just have Report Broken Site perform its function if the telemetry engine is off (or the new reporter is otherwise disabled). This will give users one consistent menu option, and reduce a fair bit of technical debt including completely removing the legacy system addon which powers Report Site Issue.
(Note that we plan to similarly remove Report Site Issue from Firefox for Android, where it is also powered by a copy of the legacy addon).
|
Assignee | |
Comment 1•8 months ago
|
||
Backed out for causing bc failures @browser_all_files_referenced.js, @browser_help_panel_cloning.js and @browser_webcompat.com_fallback.js
- Backout link
- Push with failures
- Failure Log @browser_all_files_referenced.js
- Failure Log @browser_webcompat.com_fallback.js
- Failure Log @browser_help_panel_cloning.js
|
|
Assignee | |
Comment 5•8 months ago
|
||
Only one of those failures are happening for me on my local build, and I'm not sure why that file isn't referenced, as it's not related to my patch at all. The fallback might just be an intermittent failure, so I'll try again after fixing the last of the three, which is actually failing on my local build.
The fallback was a perma but I can't confirm which push caused it beacuse there was a build bustage. I backed out 5 pushes at the same time and the fallback is not happening anymore. Maybe it was caused by one of the other pushes. Same situation for the first failure listed.
|
||
Updated•8 months ago
|
|
|
Assignee | |
Comment 7•8 months ago
|
||
Ok, i've updated the patches to fix the test which was failing for me locally, so hopefully this time it will stick.
|
||
Comment 9•8 months ago
|
||
Backed out for causing build bustages
Backout link: https://hg.mozilla.org/integration/autoland/rev/032610b8c72c48941af106b2875d12a6437a4de7
INFO - package> /builds/worker/.mozbuild/srcdirs/gecko-8a5b87fe5d69/_virtualenvs/build/bin/python -m mozbuild.action.langpack_manifest --locales en-US --app-version 138.0a1 --max-app-ver 138.0a1 --app-name 'Nightly' --l10n-basedir '/builds/worker/checkouts/l10n-central' --metadata /builds/worker/checkouts/gecko/browser/locales/en-US/langpack-metadata.ftl --langpack-eid 'langpack-en-US@firefox.mozilla.org' --input ../../dist/xpi-stage/locale-en-US
[task 2025-03-20T01:06:27.856Z] 01:06:27 ERROR - package> Traceback (most recent call last):
[task 2025-03-20T01:06:27.856Z] 01:06:27 INFO - package> File "/builds/worker/fetches/python/lib/python3.8/runpy.py", line 194, in _run_module_as_main
[task 2025-03-20T01:06:27.856Z] 01:06:27 INFO - package> return _run_code(code, main_globals, None,
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> File "/builds/worker/fetches/python/lib/python3.8/runpy.py", line 87, in _run_code
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> exec(code, run_globals)
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/action/langpack_manifest.py", line 602, in <module>
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> main(sys.argv[1:])
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/action/langpack_manifest.py", line 569, in main
[task 2025-03-20T01:06:27.857Z] 01:06:27 INFO - package> parse_chrome_manifest(
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/action/langpack_manifest.py", line 351, in parse_chrome_manifest
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> parse_chrome_manifest(
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/action/langpack_manifest.py", line 351, in parse_chrome_manifest
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> parse_chrome_manifest(
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/action/langpack_manifest.py", line 349, in parse_chrome_manifest
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> for entry in parse_manifest(None, path):
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> File "/builds/worker/checkouts/gecko/python/mozbuild/mozpack/chrome/manifest.py", line 380, in parse_manifest
[task 2025-03-20T01:06:27.858Z] 01:06:27 INFO - package> fileobj = open(path)
[task 2025-03-20T01:06:27.859Z] 01:06:27 INFO - package> FileNotFoundError: [Errno 2] No such file or directory: '../../dist/xpi-stage/locale-en-US/browser/features/chrome.manifest'
[task 2025-03-20T01:06:27.859Z] 01:06:27 ERROR - package> gmake[6]: *** [/builds/worker/checkouts/gecko/toolkit/locales/l10n.mk:205: package-langpack-en-US] Error 1
[task 2025-03-20T01:06:27.859Z] 01:06:27 INFO - package> gmake[6]: Leaving directory '/builds/worker/workspace/obj-build/browser/locales'
[task 2025-03-20T01:06:27.859Z] 01:06:27 ERROR - package> gmake[5]: *** [/builds/worker/checkouts/gecko/toolkit/locales/l10n.mk:198: langpack-en-US] Error 2
[task 2025-03-20T01:06:27.859Z] 01:06:27 INFO - package> gmake[5]: Target 'langpack' not remade because of errors.
[task 2025-03-20T01:06:27.859Z] 01:06:27 ERROR - package> gmake[4]: *** [Makefile:151: libs] Error 2
[task 2025-03-20T01:06:27.860Z] 01:06:27 ERROR - package> gmake[3]: *** [/builds/worker/checkouts/gecko/config/rules.mk:359: default] Error 2
[task 2025-03-20T01:06:27.860Z] 01:06:27 ERROR - package> gmake[2]: *** [/builds/worker/checkouts/gecko/browser/build.mk:6: package] Error 2
[task 2025-03-20T01:06:27.861Z] 01:06:27 ERROR - gmake[1]: *** [/builds/worker/checkouts/gecko/build/moz-automation.mk:102: automation/package] Error 2
[task 2025-03-20T01:06:27.861Z] 01:06:27 INFO - gmake[1]: Target 'automation/build' not remade because of errors.
[task 2025-03-20T01:06:27.864Z] 01:06:27 INFO - Stopping sccache server...
|
|
Assignee | |
Comment 10•8 months ago
|
||
:flod, removing this hunk of code fixes the above breakage causing the backout, and I'm able to mach package again, but I'm not sure if that's what we want to do here? If that code is related only to report-site-issue, then maybe it's just obsolete now?
|
||
Comment 11•8 months ago
|
||
Sorry, this is too deep in the weeds of the build system.
That code is over 9 years ago, I doubt Shane is still familiar with it, and I think Eemeli might be too new to have encountered it.
baku recently removed another system add-on, I wonder if he stumbled upon it? The alternative is checking with build system folks.
|
|
||
Comment 12•8 months ago
|
||
Note: I don't think you can remove that code, because I see a lot of XPI_NAME references, so I doubt that it's the only code relying on it.
https://searchfox.org/mozilla-central/search?q=XPI_NAME
|
|
||
Comment 13•8 months ago
|
||
For what it's worth, I can't spot a difference between removing the ifdef in jar.mn, and skipping a missing manifest in the Python script.
I'm not sure the latter is something we want, because builds would fail silently.
diff --git a/python/mozbuild/mozbuild/action/langpack_manifest.py b/python/mozbuild/mozbuild/action/langpack_manifest.py
--- a/python/mozbuild/mozbuild/action/langpack_manifest.py
+++ b/python/mozbuild/mozbuild/action/langpack_manifest.py
@@ -343,21 +343,23 @@ def convert_entry_flags_to_platform_code
# 'platforms': ['win', 'mac'],
# 'path': 'chrome/pl/locale/pl/autoconfig/'
# },
# ]
###
def parse_chrome_manifest(path, base_path, chrome_entries):
for entry in parse_manifest(None, path):
if isinstance(entry, Manifest):
- parse_chrome_manifest(
- os.path.join(os.path.dirname(path), entry.relpath),
- base_path,
- chrome_entries,
- )
+ entry_path = os.path.join(os.path.dirname(path), entry.relpath)
+ if os.path.exists(entry_path):
+ parse_chrome_manifest(
+ entry_path,
+ base_path,
+ chrome_entries,
+ )
elif isinstance(entry, ManifestLocale):
entry_path = os.path.join(
os.path.relpath(os.path.dirname(path), base_path), entry.relpath
)
chrome_entries.append(
{
"type": "locale",
"alias": entry.name,
|
||
Comment 14•8 months ago
|
||
I'm sorry, but I cannot help here. Porting the other system add-ons did not require touching these #ifdef XPI_NAME things.
|
|
||
Comment 15•8 months ago
•
|
||
Discussing it with :willdurand on Slack, and he agrees that skipping a non-existent manifest doesn't feel right.
Maybe this is sensible enough (only ignore features* missing manifests)
diff --git a/python/mozbuild/mozbuild/action/langpack_manifest.py b/python/mozbuild/mozbuild/action/langpack_manifest.py
--- a/python/mozbuild/mozbuild/action/langpack_manifest.py
+++ b/python/mozbuild/mozbuild/action/langpack_manifest.py
@@ -343,18 +343,23 @@ def convert_entry_flags_to_platform_code
# 'platforms': ['win', 'mac'],
# 'path': 'chrome/pl/locale/pl/autoconfig/'
# },
# ]
###
def parse_chrome_manifest(path, base_path, chrome_entries):
for entry in parse_manifest(None, path):
if isinstance(entry, Manifest):
+ entry_path = os.path.join(os.path.dirname(path), entry.relpath)
+ # This is needed to work around the hack in browser/locales/jar.mn
+ # from bug 1240628, when there are no localized system add-ons.
+ if entry.relpath.startswith("features") and not os.path.exists(entry_path):
+ continue
parse_chrome_manifest(
- os.path.join(os.path.dirname(path), entry.relpath),
+ entry_path,
base_path,
chrome_entries,
)
elif isinstance(entry, ManifestLocale):
entry_path = os.path.join(
os.path.relpath(os.path.dirname(path), base_path), entry.relpath
)
chrome_entries.append(
|
||
Comment 16•8 months ago
|
||
I looked into this some more today and based on what I managed to gather, I feel like it may be worth to consider removing the hack introduced by Bug 1236014, instead of adding a new workaround in langpack_manifest.py (and along with that decide if we want to close Bug 1240628 as wantfix, or keep it to track looking into a better option to include localizations in built-in addons if we need to then the current hack introduced by Bug 1236014).
As an additional side note, I'm pretty sure we would have hit this same build failure even if we would kept the list of addons in browser/features unchanged but removed locales packaged in the generated addons xpi from all of them.
Let's also ask Mike Hommey perspective before proceeding with removing the hack as part of this bugzilla issue.
Details from investigation
The following are a few more details that I gathered so far by looking more closely into the build failure, and that may be useful to consider while we are determining the appropriate next step to fix it:
What is the value of "XPI_NAME" define used in browser/locales/jar.mn?
The XPI_NAME define used by that preprocessing #ifdef XPI_NAME part of browser/locales/jar.mn seems to be set to locale-en-US when we are preprocessing that file as part of a mach package run (on a full build), the #ifdef was requested by Mike in the review of Bug 1236014 to keep a little bit more contained the hack that Shane has specifically added for an addon located in browser/features (which was Pocket at the time)
After digging into the history a bit more, it looks like XPI_NAME usage in the locales packaging phase may be "kind of side-effect" of the langpacks being packaged as xpi files.
Based on that I've been wondering if that #ifdef we added there did actually restrict that hack in practice, I'd expect XPI_NAME to be always defined and set to locale-en-US (or some other locale) while we preprocess browser/locales/jar.mn, it feels another details that may be worth double-checking with Mike Hommey to determine if that may have been actually the case.
Would be other uses of "XPI_NAME" in tree break if we remove the section conditioned on "XPI_NAME" in browser/locales/jar.mn?
XPI_NAME is definitely used elsewhere in the tree, but it did exist already before the hack introduced by Bug 1236014.
Only locales included in the browser/features/ directory could be leveraging that line in browser/locales/jar.mn, and so personally I'd not expect the other uses of XPI_NAME to be affected by the removal of that hack added by Bug 1236014, this is definitely something we should double-check with Mike Hommey.
Without the patch retiring webcompat-reporter from browser/features (the patch attached to this bugzilla issue), the generated OBJDIR/dist/xpi-stage/locale-en-US/browser/chrome.manifest jar manifest looks like this:
manifest webcompat-reporter@mozilla.org/chrome.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
How is the build issue triggered as a side effect of the hack added by Bug 1236014 on browser/locales/jar.mn?
That % manifest features/chrome.manifest line in browser/locales/jar.mn is definitely what is getting manifest features/chrome.manifest to be added into OBJDIR/dist/xpi-stage/locale-en-US/browser/chrome.manifest (as :willdurand have also tracked down and mentioned in the slack thread)
The presence of manifest features/chrome.manifest in OBJDIR/dist/xpi-stage/locale-en-US/browser/chrome.manifest is then what makes langpack_manifest.py's parse_chrome_manifest to try to recurse into the non existing features subdir looking for that additional non-existing OBJDIR/dist/xpi-stage/locale-en-US/browser/chrome.manifest file.
Would other addons bundled into Firefox need the hack added by Bug 1236014 on browser/locales/jar.mn?
We are in the process of migrating away for addons bundled in the build as unsigned xpi files (which are the xpi files that are generated as part of the build and stored in the application browser/features subdir), and bundle them into the omni jar instead.
webcompat-reporter is currently the only remaining one that was still using locales packaged into the addon itself (we have migrated the remaining localized string that were still included in formautofill to browser fluent files as part of migrating into the omni jar, and webcompat is not using any localized strings), and so there wouldn't be anything adding locales under the OBJDIR/dist/xpi-stage/locales-en-US/browser/features subdir once we retire webcompat-reporter.
|
|
||
Comment 17•8 months ago
|
||
Hi Mike,
the patch attached to this issue is retiring webcompat-reporter and this addon bundled into Firefox was now the last one still leveraging the hack introduced by Bug 1236014 on browser/locales/jar.mn. The patch did hit a build failure in autoland that seems to be a side-effect of that hack.
Based on what I gathered so far by looking more closely into it (see comment 16), I feel like it may be worth to consider removing the hack introduced by Bug 1236014 instead of adding a new workaround in langpack_manifest.py.
Based on your thoughts while reviewing Bug 1236014 (if looking back to the bugzilla comments manage to trigger some more memories about it) and knowledge about how the underlying build process works, how do you feel about the two options we determined so far? (removing the hack vs. applying a new workaround in langpack_manifest.py)
|
||
Comment 18•8 months ago
|
||
It sounds like removing the hack and wontfixing the bug it references would be the way to go.
|
|
Assignee | |
Comment 19•8 months ago
|
||
I've updated my patch to do so (basically what I did in comment #10.
:rpl, if you agree, I can try landing it again tomorrow.
|
||
Comment 20•8 months ago
|
||
|
||
Comment 21•8 months ago
|
||
Backed out for bc failures on browser_webcompat.com_fallback.js on macOS.
Failure link - fails only on mac as seen on the above link
|
|
Assignee | |
Comment 22•8 months ago
•
|
||
I can't for the life of me get this to fail on my own macbook locally, having run dozens of tests. It seems as though the problem is happening in this code:
add_task(async function testEnabledForValidURLs() {
await testMenuEnabledForValidURLs(AppMenu());
await testMenuEnabledForValidURLs(HelpMenu());
// it seems to be hanging here
await testMenuEnabledForValidURLs(ProtectionsPanel());
});
So this smells like another instance of what bit me here:
https://searchfox.org/mozilla-central/source/browser/components/reportbrokensite/test/browser/browser_report_site_issue_fallback.js#75-78
Since that is related more to the inability to test the Help menu all that reliably (requiring odd hacks), I think I'd rather disable the test here as well for now.
|
|
||
Comment 23•8 months ago
|
||
|
|
||
Comment 24•8 months ago
|
||
Backed out for causing macOS bc failures @ browser_webcompat.com_fallback.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/8f4aa283fe61f93dd2a639306e21c23a4f9bd84b
Failure log -> browser/components/reportbrokensite/test/browser/browser_webcompat.com_fallback.js
|
|
||
Comment 25•8 months ago
|
||
|
||
Comment 26•8 months ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•6 months ago
|
Description
•