701944 - need warning UI for users who run into bug 691847 that encourages a fresh install

Status: RESOLVED WONTFIX

(Firefox :: Installer, defect)

Description

[Asa Dotzler [:asa]]

(filing this bug on behalf of the 8.0.1 chemspill team)

We're crashing on startup when users use Windows System Restore and end up on a previous Firefox version.  See bug 691847.

We want to be able to identify that problem before a startup crash and raise a dialog that says "We're sorry. It appears Firefox is broken. Please click on the link below to download and install a new Firefox."

Comment 1

[Asa Dotzler [:asa]]

need L10N and UX on this to verify the string.

Comment 2

[Axel Hecht [:Pike]]

Basically, we can't get the string localized at this point, mostly because there's nothing hooked up for the release repositories.

But as we have l10n-merge on for the release builds anyway, adding this in English shouldn't require tweaking the patch that does that, if we want to have it on central etc, too.

This is a case where I excempt string freeze, as the UX is clearly worse without this patch than with this patch and the string showing up in English. Being a distinct piece of UI also helps, as it doesn't mix languages.

Once we land this, we should announce this and the rationale in .l10n.

Comment 3

[Alex Keybl [:akeybl]]

(In reply to Axel Hecht [:Pike] from comment #2)
> Basically, we can't get the string localized at this point, mostly because
> there's nothing hooked up for the release repositories.

Sorry I don't know all of the localization context to fully understand your comment. Are you saying that we can't get this localized because the string is not yet available? Or that we won't be able to localize this string ever?

Comment 4

[Axel Hecht [:Pike]]

We're not going to be able to localize this string for the chem-spill 8.0.1, and we'll have smaller coverage for 9, and possibly even 10.

Comment 5

[Justin Dolske [:Dolske]]

I'm a little confused. If users are crashing on startup, they won't be able to update to 8.0.1 (or whatever version we add this to) without a manual update, no?

From reading bug 691847 I don't understand why a prompt would be needed... Doesn't that bug fix the issue? If not, can't Firefox flash caches or whatever itself instead of asking the user for a manual reinstall?

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Justin Dolske [:Dolske] from comment #5)
> I'm a little confused. If users are crashing on startup, they won't be able
> to update to 8.0.1 (or whatever version we add this to) without a manual
> update, no?

This will not fix an 8.0.1 system restored to 7, but will prevent a similar occurrence if people do a system restore from 9.0 to 8.0.1 in the future.

> From reading bug 691847 I don't understand why a prompt would be needed...
> Doesn't that bug fix the issue? If not, can't Firefox flash caches or
> whatever itself instead of asking the user for a manual reinstall?

From our read of the fix being worked on in 691847 earlier today, it did not seem to resolve the issue. Perhaps we were mistaken, but nobody has yet corrected us.

Jeff - could you clarify if 691847 will resolve the issue of 9 downgraded to 8.0.1?

Comment 7

[Axel Hecht [:Pike]]

(In reply to Asa Dotzler [:asa] from comment #0)
> We want to be able to identify that problem before a startup crash and raise
> a dialog that says "We're sorry. It appears Firefox is broken. Please click
> on the link below to download and install a new Firefox."

PS: If Firefox is broken enough to not start up, clicking on a link won't work, right? Need a plain text url to copy and paste into IE?

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Axel Hecht [:Pike] from comment #7)
> (In reply to Asa Dotzler [:asa] from comment #0)
> > We want to be able to identify that problem before a startup crash and raise
> > a dialog that says "We're sorry. It appears Firefox is broken. Please click
> > on the link below to download and install a new Firefox."
> 
> PS: If Firefox is broken enough to not start up, clicking on a link won't
> work, right? Need a plain text url to copy and paste into IE?

Thanks for bringing this up - this was discussed today but didn't make it into the bug. We'd like to explore the possibility of forcing the link to be opened in IE (as opposed to the default browser, which may very well be Firefox), since this issue is Windows specific for now.

Comment 9

[Jeff Walden [:Waldo]]

I think it will resolve the issue as far as JS serialization compatibility goes.  However, it will not address the issue as far as all the other contents of omni.jar go -- XUL, CSS, images, &c.  The new bytecode version allows you to detect that your omni.jar is incompatible.  But it can't possibly do anything about all the other resources in omni.jar being wrong.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Jeff Walden (remove +bmo to email) from comment #9)
> I think it will resolve the issue as far as JS serialization compatibility
> goes.  However, it will not address the issue as far as all the other
> contents of omni.jar go -- XUL, CSS, images, &c.  The new bytecode version
> allows you to detect that your omni.jar is incompatible.  But it can't
> possibly do anything about all the other resources in omni.jar being wrong.

I understand now. We need to discuss simple low risk methods of determining whether an omni.jar file is out of sync.

Comment 11

[Jeff Walden [:Waldo]]

I've started looking into this, but I should make a strong disclaimer: once the JS engine does a bytecode version check as bug 691847's patch has it do, the issue is more the provenance of the JS component loader, the JS subscript loader, and the startup cache.  Someone who knows that code would be a much better point man on this, and would be much more knowledgeable about any issues.

That said, I can follow control flow and call graphs with MXR as well as the next man, and I think I might have at least a plausible start at what an implementation would look like.  It appears to me that the right place to put a precautionary check is probably somewhere in js/xpconnect/loader/mozJSLoaderUtils.cpp in ReadCachedScript.  (Right at startup cache initialization would be better, but at that point there's no JSContext to use to check the bytecode version, and spinning one up using JSAPI primitives would do things like touch per-thread data and other things that seem pretty risky.)  Dumping this at the start of that method would -- I think -- maybe -- do the job.  I am making some assumptions here that should really really really be verified by a startupcache/omnijar person:

  static bool haveDoneBadBytecodeCheck = false;
  if (!haveDoneBadBytecodeCheck) {
    haveDoneBadBytecodeCheck = true;

    JSScript *dummy = NULL;
    NS_NAMED_LITERAL_CSTRING(xpcomUtils, "jsloader/resource/gre/modules/XPCOMUtils.jsm");
    if (!ReadCachedScript(cache, xpcomUtils, cx, &dummy))
      FailWithReinstallWarningAndDie();
  }

I'm using XPCOMUtils.jsm only because when I debugged into a crash with MSVC, that seemed to be the script that was dodgy, so I'm guessing I can "rely" on it always being there.  I'm also manually PathifyURI'ing it because creating a URI for it seems like unwanted chance for error.  (I totally cargo-culted the pathified URI from a comment in StartupCacheUtils.cpp.)

I don't know if ReadCachedScript is as early as things go awry -- if other non-script stuff gets read out of the cache earlier, its contents could also cause things to go awry.  But that's a rather speculative fear, based mostly on my not knowing the first thing about the startup cache or about omnijar.

Anyway.  There's a start at it.  But you really don't want me driving this aspect of it if at all possible.

Comment 12

[Jeff Walden [:Waldo]]

Er, that would be |if (!NS_FAILED(ReadCachedScript(...)))|, probably.

Comment 13

[juan becerra [:juanb]]

I just noticed that if I perform the steps to reproduce using upgrading from 6.0.1 to 8.0, I don't even get a crash. The process doesn't seem to be launched. Could this prevent fx from showing anything at all, and does it make this fix a little more complex?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Yes, all bets are off if we're mixing and matching random versions of omni.jar and binary components. Implementing a generic defense against pieces of the install being corrupted is error-prone and generally not something we should be considering in a chemspill, I think.

If we really need to mitigate the impact of this scenario in a chemspill, bug 701875 might be the more reasonable patch to take.

Comment 15

[Alex Limi (:limi) — Firefox UX Team]

(In reply to Asa Dotzler [:asa] from comment #0)
> We want to be able to identify that problem before a startup crash and raise
> a dialog that says "We're sorry. It appears Firefox is broken. Please click
> on the link below to download and install a new Firefox."

As long as we make sure to launch this with IE (on Windows) and Safari (should this ever apply to Mac), this looks good to me.

Comment 16

[chris hofmann]

This probably needs security review as well.  

Setting precedence with a dialog with this kind of message, and redirecting users to download software, sounds pretty scary.   

It's likely to be spoofed in the future and lead to installation of rouge software.  

If we can think of ways that allow users to authenticate the source of the message and the source of download package we should be trying to do that.

Comment 17

[Michael Wu [:mwu]]

Attachment: Check buildid

This patch does the checking part but not the UI/notification part. I haven't tested whether it works properly after l10n repacking. It embeds the buildid into the omnijar and checks the buildid right after opening the omnijar and before any real data is read from the omnijar.

Comment 18

[Michael Wu [:mwu]]

Oh, and this patch also some additional packaging parts to not break Android. I can test and polish this part if someone can put together the windows dialog part.

Comment 19

[Jim Mathies [:jimm]]

From reading everything over, it appears this has always been a problem in our update process, we just happened to pick a really bad day to do an update. Now that day has passed, so presumably the problem for the rest of the pre-update users reverts back to what it was previously, which I think we have established was a rare problem. Also with bug 701875 fixed, it sounds like this will never be a problem again. So I’m wondering if people still feel this “bug” needs fixing? Seems like it may not.

Comment 20

[Christopher Blizzard (:blizzard)]

We need to do a lot more work to make Firefox survive a system restore and it's something we're going to have to roll out over a couple of releases.  The bugs we have on file don't cover enough and we're likely to end up with a frankeninstall which is - at least in my opinion - worse than a message that tells someone how to get back to a good state.

Our hope was that a warning would at least give users a chance to know what to do in the 8 -> 9 transition and however many releases it takes us to get to a full fix.

So this is tactical, but very useful for sure.

And so far this isn't a rare event, it was just hidden from view since we didn't have data on it.  The rate of crash/failure per upgrade doesn't seem to be decreasing much right now, but we'll know more tomorrow based on today's experiments.

Comment 21

[Alex Keybl [:akeybl]]

I would really like bug 701875 to be the correct fix, but I'm not sure that we yet understand what regressions it may cause. For instance, does renaming omni.jar affect builds in releng or possibly add-on compatibility?

Until then, I'd like to continue evaluating the risk of this solution to leave our options open.

Comment 22

[Jim Mathies [:jimm]]

Attachment: dialog patch v.1

This is a starting point. I've added a dialog template to the widget.rc which embeds in xul.dll plus the code to display it. Additional localizations can also be defined here as well, or we can load strings dynamically from some where and set them via code. This doesn't have the download link in it yet, which will require a bit of string/click trickery.

Comment 23

[Michael Wu [:mwu]]

(In reply to Alex Keybl [:akeybl] from comment #21)
> I would really like bug 701875 to be the correct fix, but I'm not sure that
> we yet understand what regressions it may cause. For instance, does renaming
> omni.jar affect builds in releng or possibly add-on compatibility?
> 

I do not expect any regressions from renaming omni.jar.

Comment 24

[Jim Mathies [:jimm]]

A couple open questions - 

1) is this fx specific, or will we need to break up resources based on the app being built?

2) localization of strings - how we handle it has an impact on how we create and display the dialog. Can we open up string bundles here or is that functionality not working?

Comment 25

[Axel Hecht [:Pike]]

As per face-to-face talk with legneato, this is not on the critical path for 8.0.x.

For a real UX, he had more comments, but I'll let him chime in here in case there's more back channel dialog.

For the strings, they should probably be next or in updater.ini, and read from that ini file in a similar fashion to what the updater does, possibly with en-US fallbacks hard-coded in case that file is corrupt. Other fallbacks are not required.

Comment 26

[Jim Mathies [:jimm]]

Attachment: base patch v.1

Comment 27

[Jim Mathies [:jimm]]

Attachment: win patch v.1

Comment 28

[Jim Mathies [:jimm]]

Attachment: dialog

Before I start filling in on the string stuff, we need to figure out what we want this dialog to look like, and what we want it to say. This is just a rough cut on what we might put in here. One caveat, if we do add linked text to launch IE, it should be stand alone in the dialog and not embedded in a paragraph of text. Static text controls are rectangular in shape and don't overlap well, so breaking these up from other text makes the coding much simpler.

I can also add an icon if we want which would need to be embedded in xul or the exe, and I can bold/italics/change font sizes similar to what we do on the crash reporter dialog.

I also think someone mentioned this would need a security review. I'll kick that off once we have a better idea on look and feel.

Comment 29

[Alex Keybl [:akeybl]]

We're no longer considering this for 8.0.1 - we've decided any of the options that mitigate bug 691847 are too high risk for a chemspill.

Rob is going to lead the effort on FF9 code changes associated with system restores. Please sync up with him to see if this option is still on the table.

Thanks for putting together the work so far!

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 574446 [details]
dialog

(In reply to Alex Keybl [:akeybl] from comment #29)
> We're no longer considering this for 8.0.1 - we've decided any of the
> options that mitigate bug 691847 are too high risk for a chemspill.
> 
> Rob is going to lead the effort on FF9 code changes associated with system
> restores. Please sync up with him to see if this option is still on the
> table.
> 
> Thanks for putting together the work so far!

Ok, putting additional development on hold until those decisions get made.