Closed Bug 1403592 Opened 7 years ago Closed 7 years ago

stylo: panicked at 'already immutably borrowed'

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

testcase.html
369 bytes, text/html
Details
Bug 1403592: Never flush the user font set when getting font metrics from style resolution.
59 bytes, text/x-review-board-request
manishearth
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1403592: Crashtest.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Attached file testcase.html — 
The attached testcase panics in m-c rev 20170926-7d15bc419c6c

thread '<unnamed>' panicked at 'already immutably borrowed', /builds/worker/workspace/build/src/third_party/rust/atomic_refcell/src/lib.rs:198                                                                                       [159/1854]
#0: mozalloc_abort, at memory/mozalloc/mozalloc_abort.cpp:33
#1: abort, at memory/mozalloc/mozalloc_abort.cpp:80
#2: panic_abort::__rust_start_panic, at src/libpanic_abort/lib.rs:61
#3: std::panicking::rust_panic, at src/libstd/panicking.rs:580
#4: std::panicking::rust_panic_with_hook, at src/libstd/panicking.rs:565
#5: std::panicking::begin_panic<collections::string::String>, at src/libstd/panicking.rs:511
#6: std::panicking::begin_panic_fmt, at src/libstd/panicking.rs:495
#7: atomic_refcell::AtomicBorrowRefMut::new, at obj-firefox/toolkit/library/gtest/rust/<panic macros>:8
#8: atomic_refcell::AtomicRefCell<style::gecko::data::PerDocumentStyleDataImpl>::borrow_mut<style::gecko::data::PerDocumentStyleDataImpl>, at third_party/rust/atomic_refcell/src/lib.rs:97
#9: style::gecko::data::PerDocumentStyleData::borrow_mut, at servo/components/style/gecko/data.rs:142
#10: geckoservo::glue::Servo_StyleSet_FlushStyleSheets, at servo/ports/geckolib/glue.rs:1050
#11: mozilla::ServoStyleSet::UpdateStylist, at layout/style/ServoStyleSet.cpp:1433
#12: mozilla::ServoStyleSet::AppendFontFaceRules, at layout/style/ServoStyleSet.cpp:1390
#13: nsIDocument::FlushUserFontSet, at layout/style/StyleSetHandleInlines.h:319
#14: nsIDocument::GetUserFontSet, at dom/base/nsDocument.cpp:13409
#15: nsRuleNode::GetMetricsFor, at layout/style/nsRuleNode.cpp:391
#16: Gecko_GetFontMetrics, at layout/style/ServoBindings.cpp:2485
#17: style::gecko::wrapper::{{impl}}::query, at servo/components/style/gecko/wrapper.rs:851
#18: style::values::specified::length::{{impl}}::reference_font_size_and_length::query_font_metrics, at servo/components/style/values/specified/length.rs:124
#19: style::values::specified::length::FontRelativeLength::reference_font_size_and_length, at servo/components/style/values/specified/length.rs:169
#20: style::values::specified::length::FontRelativeLength::to_computed_value, at servo/components/style/values/specified/length.rs:109
#21: style::values::computed::length::{{impl}}::to_computed_value, at servo/components/style/values/computed/length.rs:34
#22: style::values::computed::length::{{impl}}::to_computed_value, at servo/components/style/values/computed/length.rs:515
#23: style::properties::longhands::margin_left::cascade_property, at 4c5e399ab7a3687bb678171e1947c4c6e67e4e8b18b790cd09d14d9b64d11b65de9de42106af385d2ea0fd26d312a9bc18aa9566af2e1766945cfc4d52998e2e/toolkit/library/x86_64-unknown-linux-gnu/
debug/build/style-bccbf2b1c0916c0e/out/properties.rs:33599
#24: style::properties::apply_declarations<closure,core::iter::FlatMap<style::rule_tree::SelfAndAncestors, core::iter::FilterMap<core::iter::Rev<style::properties::declaration_block::DeclarationImportanceIterator>, closure>, closure>>, at
4c5e399ab7a3687bb678171e1947c4c6e67e4e8b18b790cd09d14d9b64d11b65de9de42106af385d2ea0fd26d312a9bc18aa9566af2e1766945cfc4d52998e2e/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:143127
#25: style::properties::cascade, at 4c5e399ab7a3687bb678171e1947c4c6e67e4e8b18b790cd09d14d9b64d11b65de9de42106af385d2ea0fd26d312a9bc18aa9566af2e1766945cfc4d52998e2e/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0
e/out/properties.rs:142737
#26: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:586
#27: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style_and_visited<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:309
#28: style::style_resolver::{{impl}}::cascade_style_and_visited_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:279
#29: style::style_resolver::with_default_parent_styles<style::gecko::wrapper::GeckoElement,closure,style::style_resolver::ResolvedStyle>, at servo/components/style/style_resolver.rs:106
#30: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style_and_visited_with_default_parents<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:278
#31: geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement, at servo/ports/geckolib/glue.rs:745
#32: mozilla::ServoStyleSet::GetBaseContextForElement, at layout/style/ServoStyleSet.cpp:1237
#33: nsComputedDOMStyle::DoGetStyleContextNoFlush, at layout/style/nsComputedDOMStyle.cpp:717
#34: nsSMILCompositor::ComposeAttribute, at layout/style/nsComputedDOMStyle.h:121
#35: nsSMILAnimationController::DoSample, at dom/smil/nsSMILAnimationController.cpp:455
#36: nsSMILTimeContainer::Sample, at dom/smil/nsSMILTimeContainer.cpp:185
#37: nsSMILAnimationController::WillRefresh, at dom/smil/nsSMILAnimationController.cpp:167
#38: nsRefreshDriver::Tick, at layout/base/nsRefreshDriver.cpp:1886
#39: mozilla::RefreshDriverTimer::TickRefreshDrivers, at layout/base/nsRefreshDriver.cpp:307
#40: mozilla::RefreshDriverTimer::Tick, at layout/base/nsRefreshDriver.cpp:329
#41: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver, at layout/base/nsRefreshDriver.cpp:770
#42: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync, at layout/base/nsRefreshDriver.cpp:584
#43: mozilla::layout::VsyncChild::RecvNotify, at layout/ipc/VsyncChild.cpp:67
#44: mozilla::layout::PVsyncChild::OnMessageReceived, at 9a6479c28b0f57ad4e6d1aec1c5258678abc48225caeaa2a25f8a6893b83595687422ba915c00ef9e645c78dda14478aa8e2f891ae54d4764ccdba97f65bc47d/ipc/ipdl/PVsyncChild.cpp:155
#45: mozilla::ipc::MessageChannel::DispatchAsyncMessage, at ipc/glue/MessageChannel.cpp:2119
#46: mozilla::ipc::MessageChannel::DispatchMessage, at ipc/glue/MessageChannel.cpp:2049
#47: mozilla::ipc::MessageChannel::RunMessage, at ipc/glue/MessageChannel.cpp:1895
#48: mozilla::ipc::MessageChannel::MessageTask::Run, at ipc/glue/MessageChannel.cpp:1928
#49: nsThread::ProcessNextEvent, at xpcom/threads/nsThread.cpp:1039
#50: NS_ProcessNextEvent, at xpcom/threads/nsThreadUtils.cpp:522
#51: mozilla::ipc::MessagePump::Run, at ipc/glue/MessagePump.cpp:97
#52: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#53: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#54: nsBaseAppShell::Run, at widget/nsBaseAppShell.cpp:158
#55: XRE_RunAppShell, at toolkit/xre/nsEmbedFunctions.cpp:880
#56: mozilla::ipc::MessagePumpForChildProcess::Run, at ipc/glue/MessagePump.cpp:269
#57: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#58: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#59: XRE_InitChildProcess, at toolkit/xre/nsEmbedFunctions.cpp:705
#60: content_process_main, at ipc/contentproc/plugin-container.cpp:63


Changing the function to be called on 'window.onload' instead of 'requestIdleCallback' causes a different assertion:

Assertion failure: ipcDoc, at /builds/worker/workspace/build/src/accessible/generic/Accessible.cpp:859
#0: mozilla::a11y::Accessible::HandleAccEvent, at mfbt/AlreadyAddRefed.h:106
#1: mozilla::a11y::AccessibleWrap::HandleAccEvent, at accessible/atk/AccessibleWrap.cpp:1187
#2: nsEventShell::FireEvent, at accessible/base/nsEventShell.cpp:45
#3: mozilla::a11y::DocAccessible::NotifyOfLoading, at accessible/generic/DocAccessible.cpp:1480
#4: mozilla::a11y::DocManager::OnStateChange, at accessible/base/DocManager.cpp:310
#5: nsDocLoader::DoFireOnStateChange, at uriloader/base/nsDocLoader.cpp:1320
#6: nsDocLoader::FireOnStateChange, at uriloader/base/nsDocLoader.cpp:1283
#7: nsDocLoader::doStartDocumentLoad, at uriloader/base/nsDocLoader.cpp:783
#8: nsDocLoader::OnStartRequest, at uriloader/base/nsDocLoader.cpp:456
#9: mozilla::net::nsLoadGroup::AddRequest, at netwerk/base/nsLoadGroup.cpp:510
#10: nsBaseChannel::AsyncOpen, at netwerk/base/nsBaseChannel.cpp:714
#11: nsBaseChannel::AsyncOpen2, at netwerk/base/nsBaseChannel.cpp:730
#12: nsURILoader::OpenURI, at uriloader/base/nsURILoader.cpp:840
#13: nsDocShell::DoChannelLoad, at docshell/base/nsDocShell.cpp:11715
#14: nsDocShell::DoURILoad, at docshell/base/nsDocShell.cpp:11520
#15: nsDocShell::InternalLoad, at docshell/base/nsDocShell.cpp:10857
#16: nsDocShell::LoadHistoryEntry, at docshell/base/nsDocShell.cpp:12901
#17: nsDocShell::Reload, at docshell/base/nsDocShell.cpp:5530
#18: mozilla::dom::Location::Reload, at dom/base/Location.cpp:875
#19: mozilla::dom::LocationBinding::reload, at dom/base/Location.h:56
#20: mozilla::dom::GenericBindingMethod, at dom/bindings/BindingUtils.cpp:3055
#21: js::CallJSNative, at js/src/jscntxtinlines.h:293
Flags: in-testsuite?
Ugh, this looks bad, we're flushing stylesheets from style resolution, wtf?
Assignee: nobody → emilio
Priority: -- → P2
Comment on attachment 8912776 [details]
Bug 1403592: Crashtest.

https://reviewboard.mozilla.org/r/184088/#review189338
Attachment #8912776 - Flags: review?(manishearth) → review+
Comment on attachment 8912772 [details]
Bug 1403592: Never flush the user font set when getting font metrics from style resolution.

https://reviewboard.mozilla.org/r/184082/#review189340
Attachment #8912772 - Flags: review?(manishearth) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 862aa3c61098 -d 77229cfe153a: rebasing 423085:862aa3c61098 "Bug 1403592: Never flush the user font set when getting font metrics from style resolution. r=manishearth"
merging layout/style/ServoBindings.cpp
merging layout/style/nsRuleNode.cpp
rebasing 423086:4e742ed17303 "Bug 1403592: Crashtest. r=manishearth" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0a0cc4bb885a
Never flush the user font set when getting font metrics from style resolution. r=Manishearth
https://hg.mozilla.org/integration/autoland/rev/c050d8574203
Crashtest. r=Manishearth
Please either request uplift or set 57 status to wontfix.
Flags: needinfo?(emilio)
Comment on attachment 8912772 [details]
Bug 1403592: Never flush the user font set when getting font metrics from style resolution.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: just a one-liner that prevents some reentrancy in some edge cases.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8912772 - Flags: approval-mozilla-beta?
Comment on attachment 8912772 [details]
Bug 1403592: Never flush the user font set when getting font metrics from style resolution.

Fix a crash, taking it.
Should be in 57b5
Attachment #8912772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: