Closed
Bug 1164813
Opened 9 years ago
Closed 9 years ago
Browser crash when using search on meetup.com
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox40 | + | fixed |
firefox41 | --- | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | affected |
People
(Reporter: clement.lefevre, Assigned: birtles)
References
()
Details
(Keywords: regression)
Attachments
(7 files)
When using the browser on meetup.com website, clicking on the search button, then typing something in the field and run the search currently result in a browser crash, reproduced 100% of the time on master : Build ID 20150512010209 Build Type user Gaia Revision 6089234ace8b294a8feef064387604bae16254e3 Gaia Date 2015-05-10 13:57:12 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/502e1a5e722f Gecko Version 41.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150212.043653 Firmware Date Thu Feb 12 04:37:04 EST 2015 Bootloader L1TC000118D0
Reporter | ||
Comment 1•9 years ago
|
||
[Blocking Requested - why for this release]:
Keywords: qawanted,
regression
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Component: Gaia::Browser → DOM
Ever confirmed: true
Product: Firefox OS → Core
Comment 2•9 years ago
|
||
Updated•9 years ago
|
Flags: needinfo?(bbirtles)
Comment 3•9 years ago
|
||
Can we get a branch check?
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]: Crash, possibly exploitable. Gregor, if you can get this in gdb, can you say which memory it is we're unable to access? Am I correct that an account with meetup.com is needed to reproduce?
tracking-firefox40:
--- → ?
Keywords: regressionwindow-wanted
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Not doing reviews right now from comment #4) > [Tracking Requested - why for this release]: Crash, possibly exploitable. > > Gregor, if you can get this in gdb, can you say which memory it is we're > unable to access? > > Am I correct that an account with meetup.com is needed to reproduce? I did reproduced this bug without any account, and on Firefox OS on master. I took a look on Firefox desktop nightly to check I could reproduce it, to see if that was a Gecko related bug and was not able to. Maybe this can change depending on the OS? I was here using OS X.
Comment 6•9 years ago
|
||
Ah, I didn't see a "search" button, but I guess there's a "find" button. I likewise can't reproduce with a desktop OSX build. :(
Reporter | ||
Comment 7•9 years ago
|
||
I was meaning to say, on the mobile version of the website, the magnifying glass besides to "Best match", then input something and validate. I can do some screenshots if required.
Comment 8•9 years ago
|
||
(gdb) up #1 0xb4a3fb32 in mozilla::dom::Animation::GetCurrentTime ( this=this@entry=0xafa1d740) at ../../../dom/animation/Animation.cpp:102 102 result = mHoldTime; (gdb) p this $1 = (const mozilla::dom::Animation * const) 0xafa1d740 (gdb) p *this $2 = {<nsISupports> = { _vptr.nsISupports = 0xb6ac0118 <vtable for mozilla::CSSAnimation+8>}, <nsWrapperCache> = { _vptr.nsWrapperCache = 0xb6ac016c <vtable for mozilla::CSSAnimation+92>, mWrapper = {<js::HeapBase<JSObject*>> = {<No data fields>}, ptr = 0x0}, mFlags = 0}, mRefCnt = {mRefCntAndFlags = 7}, _mOwningThread = { mThread = 0xb364a080}, static _cycleCollectorGlobal = { <nsXPCOMCycleCollectionParticipant> = {<nsScriptObjectTracer> = {<nsCycleCollectionParticipant> = { _vptr.nsCycleCollectionParticipant = 0xb697ad98 <vtable for mozilla::dom::Animation::cycleCollection+8>, mMightSkip = false}, <No data fields>}, <No data fields>}, <No data fields>}, mTimeline = {mRawPtr = 0xb03ffd90}, mEffect = {mRawPtr = 0xaf9a7880}, mStartTime = {mValue = {mIsSome = true, mStorage = {u = { mBytes = "\354\312\346\a\a\000\000", mDummy = 30197336812}}}}, mHoldTime = {mValue = {mIsSome = true, mStorage = {u = { mBytes = "\000\243\341\021\000\000\000", mDummy = 300000000}}}}, mPendingReadyTime = {mValue = {mIsSome = false, mStorage = {u = { mBytes = "\354\312\346\a\a\000\000", mDummy = 30197336812}}}}, mPreviousCurrentTime = {mValue = {mIsSome = true, mStorage = {u = { mBytes = "\000\243\341\021\000\000\000", mDummy = 300000000}}}}, mPlaybackRate = 1, mReady = {mRawPtr = 0x0}, mFinished = {mRawPtr = 0x0}, mPendingState = mozilla::dom::Animation::NotPending, ---Type <return> to continue, or q <return> to quit--- mIsRunningOnCompositor = true, mIsPreviousStateFinished = true, mFinishedAtLastComposeStyle = false, mIsRelevant = false} (gdb) p &mHoldTime $11 = (mozilla::dom::Nullable<mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> > *) 0xafa1d770 (gdb) p &result $12 = (mozilla::dom::Nullable<mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> > *) 0xbe5c2030 (gdb) p mHoldTime $13 = {mValue = {mIsSome = true, mStorage = {u = { mBytes = "\000\243\341\021\000\000\000", mDummy = 300000000}}}} Not sure if I can trust GDB here but the bt never ends. Are we exhausting the stack?
Comment 9•9 years ago
|
||
Aha! Yes, that would make sense, with the TimeDuration bit likely a red herring. So nsTransitionManager::UpdateCascadeResults calls mozilla::AnimationCollection::EnsureStyleRuleFor which calls nsAnimationManager::MaybeUpdateCascadeResults which calls nsTransitionManager::UpdateCascadeResultsWithAnimations which calls nsTransitionManager::UpdateCascadeResults. That seems pretty fishy to me.
Assignee | ||
Comment 10•9 years ago
|
||
I'll have a look next week. (I have a plan to rework this arrangement but it probably won't completely land this quarter.)
Comment 11•9 years ago
|
||
I am able to repro this bug on latest Flame v3.0 and Nexus v3.0,but can't repro on latest Flame v2.0&2.1&2.2 and Nexus 5 v2.2 build by the STR in Comment 0. Actual results: Crash report pops up after tapping the magnifying glass and tapping enter key to search. See attachment: verify_v3.0.mp4 and logcat_1741.txt Reproduce rate: 5/5 -------------------------------------------------------------------------- Device: Flame v2.0 build(Unaffected) Build ID 20150515000201 Gaia Revision 84898cadf28b1a1fcd03b726cff658de470282f0 Gaia Date 2015-04-03 21:42:36 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/449ae5977a0c Gecko Version 32.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150515.032615 Firmware Date Fri May 15 03:26:26 EDT 2015 Bootloader L1TC000118D0 Device: Flame v2.1 build(Unaffected) Build ID 20150514001201 Gaia Revision c80865cb0bf73f1b97defbc646083b404feb3ac4 Gaia Date 2015-05-12 06:26:43 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/23eb9df75991 Gecko Version 34.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150514.035735 Firmware Date Thu May 14 03:57:45 EDT 2015 Bootloader L1TC000118D0 Device: Flame v2.2 build(Unaffected) Build ID 20150514162500 Gaia Revision 8d478e4df36ace173b3e505b568f1a5077fed7c3 Gaia Date 2015-05-14 17:30:50 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/666b327287d5 Gecko Version 37.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150514.201419 Firmware Date Thu May 14 20:14:30 EDT 2015 Bootloader L1TC000118D0 Device: Flame v3.0 build(Affected) Build ID 20150514160201 Gaia Revision 8897e1810aa6426ca483269af76ce2bfd2029d25 Gaia Date 2015-05-14 18:49:06 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/59b6d856160c Gecko Version 41.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150514.192647 Firmware Date Thu May 14 19:26:58 EDT 2015 Bootloader L1TC000118D0 Device: Nexus 5 v2.2 build (Unaffected) Build ID 20150514162500 Gaia Revision 8d478e4df36ace173b3e505b568f1a5077fed7c3 Gaia Date 2015-05-14 17:30:50 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/666b327287d5 Gecko Version 37.0 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150514.214406 Firmware Date Thu May 14 21:44:23 EDT 2015 Bootloader HHZ12f Device: Nexus 5 v3.0 build (Affected) Build ID 20150514010203 Gaia Revision 338f66e6a96491d2f5854b188c6b141ceb690d97 Gaia Date 2015-05-13 14:08:45 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/1fab94ad196c Gecko Version 41.0a1 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150514.044222 Firmware Date Thu May 14 04:42:38 EDT 2015 Bootloader HHZ12f
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-master:
--- → affected
Comment 12•9 years ago
|
||
Crash report: https://crash-stats.mozilla.com/report/index/ba381a68-c5e1-4948-a6cb-2d24d2150515
Comment 13•9 years ago
|
||
Updated•9 years ago
|
QA Whiteboard: [MGSEI-Triage+]
Updated•9 years ago
|
QA Contact: pcheng
Comment 14•9 years ago
|
||
The frequency of crash drops significantly as I go back to older Central builds around beginning of April/end of March. I've seen repro rate of 1 out of ~15. At this point I'd call it impossible to find an accurate window.
QA Whiteboard: [MGSEI-Triage+] → [MGSEI-Triage+][QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Keywords: qawanted,
regressionwindow-wanted
Updated•9 years ago
|
QA Whiteboard: [MGSEI-Triage+][QAnalyst-Triage?] → [MGSEI-Triage+][QAnalyst-Triage+]
Flags: needinfo?(jmercado)
Assignee | ||
Comment 15•9 years ago
|
||
I started looking into this and so far it seems like we have the following sequence: 1. nsTransitionManager::UpdateCascadeResultsWithAnimations 2. nsTransitionManager::UpdateCascadeResults 3. AnimationCollection::EnsureStyleRuleFor [for *animations*, not throttled] 4. nsAnimationManager::MaybeUpdateCascadeResults 5. nsAnimationManager::UpdateCascadeResults 6. nsTransitionManager::UpdateCascadeResultsWithAnimations i.e. back to 1 In 4 we compare whether anim->IsInEffect() != anim->mInEffectForCascadeResults. The thinking here, as I understand it, is that animations always win (i.e. they cause transitions to not apply their results even though the transitions layer is higher in the cascade) so if there's an animation in effect and it's cached property recording if it is in effect disagrees then we need to update, hence we call 5. 5 sets anim->mInEffectForCascadeResults = anim->IsInEffect() so that the next time around we should see these things as being in sync and no longer do an update in 4. However, we never actually run 5 in this test case because when we go to a style context to pass to 5 we fail to get a frame so we skip calling 5. As a result we end up with anim->IsInEffect() becoming false but anim->mInEffectForCascadeResults remains true so we keep updating forever. That's as far as I've gotten so far. The simplest solution is probably to make 4 update mInEffectForCascadeResults when it can't get an element or a frame but there are probably a number of better ways of doing this so I'll dig into it a bit further. I also need to try to create a reduced test case for this which is going to be difficult since I can't reproduce this on a b2g desktop build even after adjusting the UA string to get the right version of the site. Longer-term I'd like to try combining the compositing work of nsTransitionManager and nsAnimationManager into a separate compositor that does all this in one pass so we can be more confident we're not going to introduce similar cases of infinite recursion.
Assignee | ||
Comment 16•9 years ago
|
||
I managed to create a reduced test case that crashes Firefox desktop nightly.
Assignee | ||
Comment 17•9 years ago
|
||
I reduced this a little further. It seems the basic conditions are: * We have both a transition and an animation on the target element (including a finished transition) * We make an (inclusive?) ancestor of the target element display:none while the animation is in progress.
Assignee | ||
Comment 18•9 years ago
|
||
This is a fairly naive attempt to fix this. I suspect it might be better to simply prune animations in display:none subtrees more aggressively. We ought to be pruning them whenever we hit CheckAnimationRule but this crash is occurring with the following stack: nsAnimationManager::MaybeUpdateCascadeResults(mozilla::AnimationCollection * aCollection) Line 241 mozilla::AnimationCollection::EnsureStyleRuleFor(mozilla::TimeStamp aRefreshTime, mozilla::EnsureStyleRuleFlags aFlags) Line 803 nsAnimationManager::UpdateStyleAndEvents(mozilla::AnimationCollection * aCollection, mozilla::TimeStamp aRefreshTime, mozilla::EnsureStyleRuleFlags aFlags) Line 211 nsAnimationManager::FlushAnimations(mozilla::css::CommonAnimationManager::FlushFlags aFlags) Line 901 nsAnimationManager::WillRefresh(mozilla::TimeStamp aTime) Line 877 nsRefreshDriver::Tick(__int64 aNowEpoch, mozilla::TimeStamp aNowTime) Line 1636 Perhaps we're not flushing animated style when we update the ancestor style to display:none? The test from this patch should be useful even if we decide on another approach.
Attachment #8606872 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 19•9 years ago
|
||
For what it's worth, it looks like this is a regression from bug 960465 but there have been a bunch of related changes around processing display:none (e.g. bug 962594) and follow ups from bug 960465 so I'm not entirely sure which is to blame.
Flags: needinfo?(bbirtles)
Given that we have a consistent repro of this crash, we should consider fixing it for firefox40. Adding a tracking flag as such.
Comment on attachment 8606872 [details] [diff] [review] Make nsAnimationManager update cached in-effect status even when there is no frame Would an alternative fix be to move the call to mPresContext->TransitionManager()-> UpdateCascadeResultsWithAnimations(aCollection); inside the if (frame)? Maybe that makes more sense. Do you know why we're still running this after the element is display:none? I thought we have code to stop animations in that case, but maybe I'm misremembering. In any case, r=dbaron either as-is or with that modification.
Attachment #8606872 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-4 from comment #21) > Comment on attachment 8606872 [details] [diff] [review] > Make nsAnimationManager update cached in-effect status even when there is no > frame > > Would an alternative fix be to move the call to > mPresContext->TransitionManager()-> > UpdateCascadeResultsWithAnimations(aCollection); > inside the if (frame)? Maybe that makes more sense. I've made that change. > Do you know why we're still running this after the element is display:none? > I thought we have code to stop animations in that case, but maybe I'm > misremembering. I didn't look into that. We only stop animations once we rebuild the animations in CheckAnimationRule. So somehow a tick is triggering FlushAnimations before that happens. It may be a case of a different refresh driver triggering the flush before we rebuild animations (similar to what we had in bug 1117603) or it may be that we're failing to update animations when we update styles in this case (i.e. in nsStyleSet::GetContext, aFlags & eDoAnimation is not true).
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7be1fe5cd273
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8606872 [details] [diff] [review] Make nsAnimationManager update cached in-effect status even when there is no frame Approval Request Comment [Feature/regressing bug #]: Probably bug 960465 [User impact if declined]: Crashes on certain content (e.g. meetup.com) [Describe test coverage new/current, TreeHerder]: 3~4 days bake time on m-c, includes crashtest [Risks and why]: Minimal risk - simply updates some state that was stale [String/UUID change made/needed]: None
Attachment #8606872 -
Flags: approval-mozilla-aurora?
Comment 26•9 years ago
|
||
Comment on attachment 8606872 [details] [diff] [review] Make nsAnimationManager update cached in-effect status even when there is no frame Test, should be safe, taking it.
Attachment #8606872 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/03b141fb2c68
status-firefox40:
--- → fixed
Flags: in-testsuite+
(In reply to Brian Birtles (:birtles) from comment #23) > (In reply to David Baron [:dbaron] ⏰UTC-4 from comment #21) > > Comment on attachment 8606872 [details] [diff] [review] > > Make nsAnimationManager update cached in-effect status even when there is no > > frame > > > > Would an alternative fix be to move the call to > > mPresContext->TransitionManager()-> > > UpdateCascadeResultsWithAnimations(aCollection); > > inside the if (frame)? Maybe that makes more sense. > > I've made that change. Hmmm. I don't think that's what I meant -- I meant just making that call *also* be conditional on if (frame).
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 29•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #28) > (In reply to Brian Birtles (:birtles) from comment #23) > > (In reply to David Baron [:dbaron] ⏰UTC-4 from comment #21) > > > Comment on attachment 8606872 [details] [diff] [review] > > > Make nsAnimationManager update cached in-effect status even when there is no > > > frame > > > > > > Would an alternative fix be to move the call to > > > mPresContext->TransitionManager()-> > > > UpdateCascadeResultsWithAnimations(aCollection); > > > inside the if (frame)? Maybe that makes more sense. > > > > I've made that change. > > Hmmm. I don't think that's what I meant -- I meant just making that call > *also* be conditional on if (frame). Oh, sorry for the misunderstanding. I see what you meant now. I haven't tested that particular approach and I hope to rework this interaction in the next few months as part of bug 1151731 (something I hope to go over with you at Whistler) so I lean towards just leaving this code as-is for now. Let me know if you think it's worth fixing though.
Flags: needinfo?(bbirtles)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•