813897 - Firefox 17 crash with remote XUL e-commerce application (many affected enterprises)

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[loic.couturier]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/536.25 (KHTML, like Gecko) Version/6.0 Safari/536.25

Steps to reproduce:

I try to access to the remote XUL application at the address http://demo.rbschange.fr/admin
It's possible to reproduce the problem if you install the extension and try to login with :
login : demo
password : demo123



Actual results:

Firefox crash


Expected results:

The back-office of RBS Change have to open.

Comment 1

[Andre Klapper]

Thanks for taking the time to report this!
Please follow the steps on http://support.mozilla.com/kb/Firefox%20crashes and report back here. Thanks!

Comment 2

[loic.couturier]

I follow the page.
I can add this informations :
- Firefox doesn't crash in Safe Mode because it's the RBS Change Manager that call the xchrome protocol
- Firefox crashes on windows too
- Firefox crashes directly when we try to call the xchrome protocol
- All system are up to date
- The problem is still present on the latest Aurora version

I sent this morning a crash report with the id bp-68682e8b-9772-4013-a03c-a63bb2121121

Comment 3

[Scoobidiver (away)]

It looks like a regression from bug 775796.

Comment 4

[Mounir Lamouri (:mounir)]

Could that have been fixed by bug 811193? Does the crash appear in Nightly?

Comment 5

[Scoobidiver (away)]

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Could that have been fixed by bug 811193? Does the crash appear in Nightly?
It crashes in 17.0 (bp-c5927857-e826-4091-bbe3-2dd502121122) and in Nightly (bp-dd20e347-18ee-4ac2-83e6-abeb42121122)

Comment 6

[loic.couturier]

Hi,
It's the first time that I post a bug on bugzilla.
If it's possible, I would like to know if you have a idea on the fix date ?
This bug is critical for our product, and I have to tell our customers what to do.
Thx

Comment 7

[Mounir Lamouri (:mounir)]

This has been caused by bug 775796. I think what very likely happens is that the principal is a system principal and we try to get the URI from it. The URI being null for the system principal, we crash.

I think the best thing to do here is to not lookup if we happen to have a system principal.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch

This is fixing the crash with a test to reproduce it.

Comment 9

[Mounir Lamouri (:mounir)]

Loïc, it is hard to know when the fix will be in a release version of Firefox. If the patch I wrote is accepted, the fix should be soon in a Nightly version and likely in an Aurora version. I can't tell for Beta and release. In my opinion, it is safe enough to go in Beta.

To prevent that kind of disagreement to happen in the future, you should be using an Aurora or a Beta version of Firefox. If we knew about the bug early enough (when Firefox 17 was in Aurora/Beta), we would have been able to fix it and prevent the bug to be there in the release version. As soon as a bug is in release version, it might take up to 3 releases to have it fixed. We have 4 versions of Firefox at the same time: Nightly, Aurora, Beta and Release; every 6 weeks, the code from a version goes to the other one, Nightly being the trunk version so when something happens on trunk, it needs 6*3 weeks at best to move to Release, unless we backport the patch to another channel.

Comment 10

[Franck Stauffer]

Hi,

This bug leaves over a hundred of our customers unable to update to the latest version of Firefox. Our customers are using our application fon a daily bases to do critical work on their e-commerce website and right now they're forced to re-install 16.0.2 and block all updates. The effect on their perception of our choice to use XUL/Firefox is kinda deleterious and it would really help if  we could have a more precise estimate of the potential release date ? Is there anything we can do to help speed up the process ?

Thx,

Franck

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

This is a very low volume crash, doesn't meet the criteria for ESR tracking. If the fix is low risk please go ahead and nominate for uplift but there's no reason to track this bug for blocking our Firefox 18 release.

Comment 12

[Franck Stauffer]

I am sorry, but I don't really get it. It's a regression, that crashes 100% of our customers, that forces them to stay on a specific non ESR version. It is admittedly a low volume crash - but having so little visibility on a regression fix makes it hard for us to keep on telling our customers that Firefox is indeed the right choice for their mission-critical e-commerce applications. Volume is certainly a way to evaluate criticality but I doubt it is viable, especially for entreprise-grade releases.  Again, we're a software editor and we know one has to make choices and establish priorities, but still, it is hard to keep the faith in Firefox in this specific case.

Comment 13

[Robert Kaiser]

Remote XUL is pretty low in priority for Mozilla as AFAIK we're actually trying to get rid of it in the long term. We urge everyone to try moving to HTML5 instead.

That said, there is a patch here waiting for review, if it gets that and the necessary approvals, the crash might get fixed in time for Firefox 18 and the 17 ESR coming in parallel to that.

Comment 14

[Franck Stauffer]

Thank you for the answer. We know that remote XUL is no longer an option in the long run and we're currently moving our soft to HTML5. Yet, our installed base won't be migrating instantly to the new version (which is scheduled in 2013 anyway) - if the patch makes it to ESR 17 it means our customers will be safe for the next 12 months. Such a move would be greatly appreciated by both our customers and our development team and certainly ease the transition.

Comment 15

[Scoobidiver (away)]

F(In reply to Franck Stauffer from comment #10)
> they're forced to re-install 16.0.2 and block all updates.
This version has known vulnerabilities. Firefox 10.0.11esr is unaffected and also 10.0.12esr, both with no known vulnerabilities, for the current workaround and a future one in case the patch won't land in 18.0 and 17.0.2esr.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 686102 [details] [diff] [review]
Patch

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

Dropping r? since Michal said he's going to update the patch. Also, please answer my question: "Is it really OK to just remove them completely, or should we just be updating them or replacing them with new tests?" I didn't look at the tests carefully so I'm not sure either way.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sorry, my previous comment was for another bug. I am not a toolkit reviewer and I don't know this code. Is there another reviewer that would be better suited for the review?

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 686102 [details] [diff] [review]
Patch

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

Brian, this is a regression from bug 775796 which you reviewed. I guess that makes you a proper reviewer for that patch.

However, if you do not feel confident, could you redirect the review to :dcamp?

Comment 19

[Mounir Lamouri (:mounir)]

Brian, any ETA for that review?

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I will do it early next week, after my blocking-basecamp work is completed.

Comment 21

[Franck Stauffer]

Hi, we still have all of our users stuck on 10 ESR because of this bug. I know you guys probably have a lot of other things to fix but well, the patch fixing this dates from november and hasn't been reviewed since then. Would it be possible to get more infos on when this is going to get a proper review ?

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Brian Smith (:bsmith) from comment #20)
> I will do it early next week, after my blocking-basecamp work is completed.

Friendly reminder regarding the review :)

Comment 23

[jean-michel.ramseyer]

Hi, 
We know that you are working on things, but our customers are stuck in 10 ESR because the bug isn't fixed even nor the patch reviewed.
So great there's others new (bugged) release of Firefox and the 10 ESR is no more easily available for our customers; they have to browse the ftp area and that's not so easy for some users.

So would it be possible to get more infos on when this is going to get a proper review?

Comment 24

[jean-michel.ramseyer]

Hello again,

So now 10 ESR has ended and is auto-updated to 17 ESR which is buggy. One more time the question is : WOULD IT BE POSSIBLE TO KNOW WHEN THIS PATCH WILL BE REVIEWED?

Just a kind of reminder the application impacted by this bug is used by our customers who are online sellers like including those shown on http://www.rbschange.fr/References/ so even if the bug doesn't impact a lot of users it has an impact on business. In this case I think this could be considered as a CRITICAL BUG.

Users were surprised when the crash appeared and now if they don't pay attention they're auto updated to this buggy 17 ESR, and they start to become angry against Firefox... This bug review becomes really ridiculous and offers a BAD PERCEPTION of Firefox to companies that are using our solution...

So today, as nobody cares about this bug review our customers are stucked in a deprecated Firefox revision on team preco and because they have no other choice. Some customers asked me if I'm serious when I'm telling they have to move back to 10 ESR ; what should I answer them?

Comment 25

[Scoobidiver (away)]

All I can tell you is that a review takes usually less than one week. As a volunteer crash triager, I've never seen that situation where a review takes more than three months especially for so few changed lines.
I think Mozilla uses this bug to unofficially drop support for XUL applications.

You should start deploying your new application based on HTML5 or find a workaround in your current XUL application.

Comment 26

[Franck Stauffer]

Ok that sounds great ;) Thanks for the feedback though. I would really appreciate, for the sake of clarity, that someone at mozilla clearly says that XUL is no longer supported. I mean, at least it would be clear for everyone who still relies on this technology. We already have taken the decision to drop XUL in the next version of our software. Unfortunately, and for the exact same reason Mozilla is providing Extended Support Releases, we need to provide a stable environment for our existing customers which does not require to move to the next version in order to get access to the app they paid for.
A clear "wontfix" is better than silence in any case especially since it takes 5 minutes to do that and clearly tells everybody that XUL is dead.

Comment 27

[Robert Kaiser]

XUL isn't dead by far. But remote XUL should be. That said, a review taking this long is highly irregular, and we should take this to someone else if bsmith is unresponsive. This kind of behavior as a reviewer is just not acceptable.

Mounir, do we have someone else who can review this? Or do we have someone who we can esalate this to?


For the record, I'm all for deprecating remote XUL even further or even unsupporting it at some point, but if so, we should do this with conscious decisions and announcements at least to the Enterprise list and not just by introducing a bug and never solving it. That's as dirty as it can get and not the Mozilla way to do things.

Comment 28

[jean-michel.ramseyer]

As we told before, the next major release will be in HTML5/AJAX but we must offer a kind of LTS/ESR support for the current version and we'll do it til 2015.

As the bug has a real impact on business, could it be considered for the current ESR?

Comment 29

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 686102 [details] [diff] [review]
Patch

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

Seems reasonable to me. I am not a toolkit reviewer though.

Comment 30

[jean-michel.ramseyer]

Thank you :bsmith for the review.

:mounir what should be done before the patch applies to nightly or next release?

Will the current Firefox ESR be fixed with this patch?

Comment 31

[Scoobidiver (away)]

A second review has been requested. I hope this one will take less than one week.
One it's reviewed, Mounir will land the patch in Nightly. Somebody will check crashes are fixed. Maybe there will be a delay of one week to see possible side effects before uplifting the fix to Aurora, Beta and ESR if release drivers think it's worth to land there (benefits vs. risks).

Comment 32

[Dave Camp (:dcamp)]

Comment on attachment 686102 [details] [diff] [review]
Patch

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

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1262,5 @@
>    NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
>    NS_ENSURE_ARG(aPrincipal);
>  
> +  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> +    return NS_OK;

*didLookup should be set to false in this case, r+ with that fix.

Comment 33

[Mounir Lamouri (:mounir)]

Comment on attachment 686102 [details] [diff] [review]
Patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/36e5a5b72325

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36e5a5b72325

Comment 35

[Mounir Lamouri (:mounir)]

Comment on attachment 686102 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: see bug comments
User impact if declined: crash
Fix Landed on Version: m-c
Risk to taking this patch (and alternatives if risky): the only code path this patch is changing is a code patch leading to a crash right now, there is no way we can make this worse with this patch...
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 686102 [details] [diff] [review]
Patch

It's early still in Beta so we'll take this uplift, and considering the impact on ESR we can take this uplift there too even though it's not a security fix. This is in no way a release blocker issue so minusing for release branch landing.

Comment 37

[Mounir Lamouri (:mounir)]

Thanks :) I actually didn't meant to push this to release. I enabled all approval flags but had no idea -release was one of them ;)

Comment 38

[Scoobidiver (away)]

Verified with the STR in comment 0 in the latest Nightly.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d203380317c
https://hg.mozilla.org/releases/mozilla-beta/rev/142d6915f8ca
https://hg.mozilla.org/releases/mozilla-esr17/rev/33df6a24ad91

Comment 40

[Scoobidiver (away)]

Verified with the STR of comment 0 in 21.0a1/20130308 and 20.0b4.