Closed Bug 1565673 Opened 5 years ago Closed 2 years ago

`about:compat` does not survive extension updates

Categories

(Web Compatibility :: Interventions, defect, P3)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: denschub, Unassigned)

References

Details

  1. Take the Release build of Firefox with WebCompat 4.3.2 built-in
  2. Update to webcompat 4.3.3 by either installing https://bugzilla.mozilla.org/attachment.cgi?id=9077742 (this XPI is signed as a system extension), or by following https://bugzilla.mozilla.org/show_bug.cgi?id=1564974#c9 and have balrog deliver the update (the latter approach likely is invalid soon when we take the xpi off the staging area)
  3. Restart the browser
  4. Try to access about:compat

You will see an Invalid URL page ("Hmm. That address doesn’t look right."), which never resolves itself, even after multiple browser restarts. :(

Tom, any idea on how we could fix that?

Flags: needinfo?(twisniewski)

Are there any console errors?

Flags: needinfo?(dschubert)

Well, yes, "TypeError: docShell.failedChannel is null" from NetErrorChild.jsm:697:30, but unfortunately, that's not really helpful, so I left it out. Nothing that could point in any direction. :(

Flags: needinfo?(dschubert)

Alright, I've been able to figure this out by reproducing the problem using a local nightly with unsigned XPIs with some debug-console.logs in them.

There are two problems. The first is that the addon does not set up its resource: URLs properly for the case where a user-installed (or Balrog-installed) XPI file is used from their profile, instead of from the bundled version with Firefox. Fixing this isn't tough, we just have to change a chunk in aboutPage.js to point to the relative location of the files in the XPI:

    resProto.setSubstitution(
      ResourceSubstitution,
      Services.io.newURI("about-compat/", null, rootURI)
    );

This removes the need to bother with a jar.mn, so we can remove that as well. I've tested this on local Linux and Fennec builds, both with their bundled addon, and also with a replacement XPI installed on top of that to simulate a Balrog-delivered update.

The second problem is that the user will have to restart their running browser before about:compat will work again. The reason for this is rather obnoxious:

  • the addon uses a process script to register the about-page handler for about:compat
  • this registration expects the about-compat files to be in the XPI's root directory, but we recently moved them to /about-compat
  • as such it breaks because the files are not where it expects them (though the addon does continue to work)
  • it is never un-registered by the addon when it unloads (I recall that being deemed to be overcomplicated at review-time when about:compat was implemented)
  • it cannot be simply replaced by a new registration (with a new CID or the old one); direct re-registrations appears to be ignored
  • it cannot be unregistered, because a reference to the old registration is required, and the newly-loading addon has no way to get one.

The net result is that if the user gets a Balrog update, they will need to restart the browser for it to pick up on the new paths for the about:compat files.

Our options are simple:

  1. ignore this, and ask users to restart if they run into such temporary breakage
  2. move the about-compat files back to the root directory of the XPI.

If we decide to pick #2 for now, and try to move the files around again in the future, we can also fix the addon to unregister the about-page handler as it unloads. This should not be too difficult, it would involve some message-passing.

I have a candidate patch ready which moves the files back, and fixes the resource-URL issue. I've tested it on Fennec and Linux, and it now seems to work fine with and without a restart.

Flags: needinfo?(twisniewski) → needinfo?(dschubert)

Tom, are we ready to land your fix in m-c?

Flags: needinfo?(twisniewski)

I believe so, but I've left it up to Dennis as to when we actually do land the fix.

Flags: needinfo?(twisniewski)

Hi Tania, Andrei, can we add this scenario to our QA testing?

Flags: needinfo?(tmaity)
Flags: needinfo?(andrei.vaida)

OK, one last question -- what's the risk for landing this fix via a balrog update? Will it work?

Flags: needinfo?(twisniewski)

If we just want to ship the jar-file fix on the version 4.4.0 of the addon that's currently in m-c, and not all of the other stuff including the files moving around into an about-compat subdirectory, then the risk is quite low. At least, I've manually tested that just installing a version 4.4.1 of the XPI with only that fix into my profile doesn't break anything, before or after a Firefox restart.

However, if we want to also Balrog-deploy a version including the other fixes on GitHub (with the ones which move the files around), then we run into the problem where users will have to restart Firefox before about:compat will work again. That's the only risk I'm aware of.

Flags: needinfo?(twisniewski)

I've left it up to Dennis as to when we actually do land the fix.

I have no strong opinion here. If noone else does it, I'll make sure to build a patch merging everything we have right now (maybe including the new console logging, if it's done by then) by the end of the week!

Flags: needinfo?(dschubert)

(In reply to Ritu Kothari (:ritu) from comment #7)

Hi Tania, Andrei, can we add this scenario to our QA testing?

Of course. Emil is currently looking at the information available here and will add a regression test.

Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)

Added a regression test case to our Web Compatibility test suite.

Flags: needinfo?(tmaity)
Flags: needinfo?(emil.ghitta)

Hi Andrei, this change/fix is also being pushed in SAO update: webcompat system addon 5.0.2. I am sure your team is also testing that fix. It's a critical one to validate. I just wanted to flag it again in case the test case mentioned in c12 isn't enough to test the SAO fix. Thanks!

Flags: needinfo?(andrei.vaida)

Hi Ritu, per Bug 1568636 Comment 26, this has been verified by the Web Compatibility team.

Flags: needinfo?(andrei.vaida)
Severity: normal → S3
Priority: -- → P3

Dennis, could you confirm if this is still something we need to be worried about?

Flags: needinfo?(dschubert)

I did some tests, and surprisingly, it works now. I didn't run the full staging-rollout process, but if it works with a locally built .xpi and an xpi built by CI, I see no reason why this shouldn't work otherwise.

Changes are even visible on about:compat without restarting the browser. So, uh, good. :)

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(dschubert)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.