Created attachment 810872[details][diff][review]
patch
this is a regression from bug 471227, so rather old.
We end up calling nsOfflineCachePendingUpdate::OnStateChange nestedly, so that
mService->Schedule calls it again. The object is kept alive on stack, but
since NS_RELEASE_THIS() is called more than once, calling Release when
the strong pointer on the stack goes away crashes.
Couldn't figure out anything simpler given the odd refcnt handling.
(I think nsDocLoader should just keep this kind of webprogresslisteners alive in a
strong array, but that would require quite a bit more changes and wouldn't be
safe enough for branches.)

Comment on attachment 810872[details][diff][review]
patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 471227
User impact if declined: Crashes
Testing completed (on m-c, etc.): NA
Risk to taking this patch (and alternatives if risky): Should be very safe. Adding a flag that we don't do a thing twice
String or IDL/UUID changes made by this patch: NA
How easily could an exploit be constructed based on the patch?
Note sure about the exploit but crash might be quite easy to get.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Unfortunately the patch is very clear what kind of problem this is about.
Which older supported branches are affected by this flaw?
all
Do you have backports for the affected branches?
The patch seems to apply cleanly to branches
How likely is this patch to cause regressions; how much testing does it need?
Should be very safe.
So, in other words, I think this could land as late as possible of a cycle.

I have no problems sec-approval+'ing this but I want release management to weigh in since we'll need to take this on Aurora and Beta (and ESR24 and maybe ESR17, which is still supported). What I need from them specifically is how late they are comfortable checking this in since it clearly demonstrates the issue. Since this is a safe fix, it shouldn't affect stability really.
I assuming B2G is affected too.

We should be able to land this on 10/17 to get it into our second-to-last Beta (and then elsewhere too) to minimize exposure but also give one extra release's buffer if we needed to backout for any reason - though this is expected to be safe.

Comment on attachment 810872[details][diff][review]
patch
Setting unset branch approval flags and giving sec-approval+.
We should take this on ESR17 too if the patch applies since we are still supporting that for another two releases.