Crash in IsWindowColorDark

RESOLVED FIXED in Firefox 55

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: calixte, Assigned: masayuki)

Tracking

(Blocks 1 bug, {crash, regression})

55 Branch
mozilla55
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52- wontfix, firefox53 unaffected, firefox54- wontfix, firefox55+ fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-88a99c64-da13-41fd-97bc-8a2dd0170528.
=============================================================

There is 1 crash in nightly 55 with buildid 20170527030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1366140.

[1] https://hg.mozilla.org/mozilla-central/rev?node=f32c73555c98ab565cb7ae10a761d6817aa5c89a
Flags: needinfo?(masayuki)
Ah, this must be a simple bug of the patch. Taking.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8872272 [details]
Bug 1368318 Call TSFTextStore::ThinksHavingFocus() before clearing sEnabledTextStore since the method refers it

https://reviewboard.mozilla.org/r/143762/#review147484
Attachment #8872272 - Flags: review?(m_kato) → review+

Comment 4

2 years ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/024c52408261
Call TSFTextStore::ThinksHavingFocus() before clearing sEnabledTextStore since the method refers it r=m_kato

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/024c52408261
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
AFAICT, we'll want this on Beta/ESR52 as well. Please nominate accordingly :)
Yep. But let's watch if it's actually fixed by the patch before that. (Note that it could be just a bug of TSF or MS-IME.)
Wait, looks like that this bug is not a recent regression. I see same crash reports in 53 and older:
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=IsWindowColorDark&date=%3E%3D2016-11-30T04%3A31%3A21.000Z&date=%3C2017-05-31T04%3A31%3A21.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

So, I guess that my patch for this bug may has never fixed the crash yet. However, my patch actually fixes a regression of bug 1366140. So, anyway, we should take it to the branches, though.
Flags: needinfo?(masayuki)
Comment on attachment 8872272 [details]
Bug 1368318 Call TSFTextStore::ThinksHavingFocus() before clearing sEnabledTextStore since the method refers it

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

The crash might not have been fixed by this patch, though (see previous comment), but this fixes an actual regression of the patch for sec-high bug.

User impact if declined:

Some TSF objects are not released when every editor loses focus.

Fix Landed on Version:

55

Risk to taking this patch (and alternatives if risky):

No, really simple.

String or UUID changes made by this patch: 

No.

Approval Request Comment
[Feature/Bug causing the regression]:

Although, this might have not fixed the crash yet (see my previous comment). But actually fixes a regression of bug 1366140.

[User impact if declined]:

Some TSF objects are not released when every editor loses focus.

[Is this code covered by automated tests?]:

No.

[Has the fix been verified in Nightly?]:

Any visible symptom is there. So, cannot verify this unless if this actually fixes the crash and crash reports won't come.

[Needs manual test from QE? If yes, steps to reproduce]:

No.

[List of other uplifts needed for the feature/fix]:

Depends on the patch for bug 1366140 due to fixing its regression.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

See the patch. Really simple mistake.

[String changes made/needed]:

No.
Attachment #8872272 - Flags: approval-mozilla-esr52?
Attachment #8872272 - Flags: approval-mozilla-aurora?
> Flags: approval-mozilla-aurora?

Oops, I meant Flags: approval-mozilla-beta?... But I assume that drivers will handle it as expected ;-)
Attachment #8872272 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8872272 [details]
Bug 1368318 Call TSFTextStore::ThinksHavingFocus() before clearing sEnabledTextStore since the method refers it

The crash volume for this signature is extremely low, the other aspect of not cleaning up references in TSFTextStore while valid is not severe enough either. If it was a significant memleak, we would have heard. Given this data, I don't feel the need to uplift this fix so late in 54 or ESR52.2.
Attachment #8872272 - Flags: approval-mozilla-esr52?
Attachment #8872272 - Flags: approval-mozilla-esr52-
Attachment #8872272 - Flags: approval-mozilla-beta?
Attachment #8872272 - Flags: approval-mozilla-beta-
(In reply to Ritu Kothari (:ritu) from comment #11)
> Comment on attachment 8872272 [details]
> Bug 1368318 Call TSFTextStore::ThinksHavingFocus() before clearing
> sEnabledTextStore since the method refers it
> 
> The crash volume for this signature is extremely low, the other aspect of
> not cleaning up references in TSFTextStore while valid is not severe enough
> either. If it was a significant memleak, we would have heard. Given this
> data, I don't feel the need to uplift this fix so late in 54 or ESR52.2.

Hmm, but it fixes a recent regression of bug 1366140 obviously. (I'm not sure if it actually fixes the crash bug, though, because same signature reports exist before bug 1366140 fix.)

We'll destroy TSF objects without notifying TSF/TIP of losing focus. That must be risky... (The patch should've been included in the patch for bug 1366140.)
Setting ESR52 to wontfix based on comment 12. No signs of ESR52 crashes in the last 3 months anyway. OTOH, 55 Beta builds are still hitting crashes with this signature :(
You need to log in before you can comment on or make changes to this bug.