Closed Bug 1310885 Opened 8 years ago Closed 8 years ago

stylo: keep CSS images alive while their corresponding rule is in the style sheet

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: heycam, Assigned: u459114)

References

Details

(Whiteboard: leave-open)

Attachments

(12 files, 2 obsolete files)

Bug 1310885 - Part 3. (gecko) Create a new API to receive cached ImageValue from stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 1. (gecko) Export RefPtr<ImageValue> from gecko to stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 11. (gecko) Clean up unused things.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 0. (gecko) Export mozilla::css::ImageValue to stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 2. (stylo) Export RefPtr<ImageValue> from gecko to stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 10. (gecko) Always release ImageValue on the main thread.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 4. (gecko) Allow ImageValue::Intialize be called more then once, but only LoadImage in the first call.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 5. (stylo) Let SpecifiedUrl be able to carry ImageValue.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 6. (stylo) Pass LayerImage's image_value to gecko
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 7. (stylo) Pass content's image_value to gecko.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 8. (stylo) Pass cursor's image_value to gecko.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310885 - Part 9. (stylo) Pass style list's image_value to gecko.
59 bytes, text/x-review-board-request
heycam
: review+
Details
In nsStyleImageRequest::Resolve, there is the comment: // For now, just have unique nsCSSValue/ImageValue objects. We should // really store the ImageValue on the Servo specified value, so that we can // share imgRequestProxys that come from the same rule in the same // document. If we have a rule that starts matching, which causes a CSS image load, and which then no longer matches, Gecko will keep the imgRequestProxy alive, since it is stored on the css::ImageValue (the specified value for the properties that had the url() value). We should do something similar, so that we don't risk re-requesting images from the network when the rule starts matching again (which often happens with e.g. :hover rules).
Priority: -- → P2
Assignee: nobody → cku
Three more place need to be modified: 1. Gecko_SetCursorImage - for cursor style image 2. Gecko_SetContentDataImage - for conter stye image 3. Gecko_SetListStyleImage - for style image list 4. After fix #1 ~ #3, we can remove CreateStyleImageRequest(aModeFlags, aURI)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #10) > Three more place need to be modified: > 1. Gecko_SetCursorImage - for cursor style image > 2. Gecko_SetContentDataImage - for conter stye image > 3. Gecko_SetListStyleImage - for style image list > 4. After fix #1 ~ #3, we can remove CreateStyleImageRequest(aModeFlags, aURI) It's simpler if we handle these properties in a follow-up bug.
Attachment #8863640 - Flags: review?(cam)
Attachment #8862812 - Flags: review?(cam)
Attachment #8863641 - Flags: review?(cam)
Attachment #8862811 - Flags: review?(cam)
Attachment #8864431 - Flags: review?(cam)
Attachment #8862813 - Flags: review?(cam)
Attachment #8862814 - Flags: review?(cam)
Attachment #8863642 - Flags: review?(cam)
Attachment #8862815 - Flags: review?(cam)
Attachment #8862813 - Attachment is obsolete: true
Attachment #8862813 - Flags: review?(cam)
Attachment #8862814 - Attachment is obsolete: true
Attachment #8862814 - Flags: review?(cam)
Attachment #8865140 - Flags: review?(cam)
Attachment #8865141 - Flags: review?(cam)
Attachment #8865142 - Flags: review?(cam)
Attachment #8865143 - Flags: review?(cam)
Attachment #8865144 - Flags: review?(cam)
Comment on attachment 8863640 [details] Bug 1310885 - Part 0. (gecko) Export mozilla::css::ImageValue to stylo. https://reviewboard.mozilla.org/r/135440/#review139826
Attachment #8863640 - Flags: review?(cam) → review+
Comment on attachment 8862812 [details] Bug 1310885 - Part 1. (gecko) Export RefPtr<ImageValue> from gecko to stylo. https://reviewboard.mozilla.org/r/134732/#review139828
Attachment #8862812 - Flags: review?(cam) → review+
Comment on attachment 8863641 [details] Bug 1310885 - Part 2. (stylo) Export RefPtr<ImageValue> from gecko to stylo. https://reviewboard.mozilla.org/r/135442/#review139830
Attachment #8863641 - Flags: review?(cam) → review+
Comment on attachment 8862811 [details] Bug 1310885 - Part 3. (gecko) Create a new API to receive cached ImageValue from stylo. https://reviewboard.mozilla.org/r/134730/#review139832 ::: commit-message-12301:4 (Diff revision 5) > +1. Implement a new constructor for nsStyleImageRequest to receive a existed > +ImageValue from the caller. s/a existed/an existing/ ::: commit-message-12301:6 (Diff revision 5) > +Bug 1310885 - Part 3. (gecko) Create a new API to receive cached ImageValue from stylo. > + > +This patch implement several things: > +1. Implement a new constructor for nsStyleImageRequest to receive a existed > +ImageValue from the caller. > +2. Implement Gecko_ImageValue_Create to allow stylo create a gecko::ImageValue s/create/to create/ ::: commit-message-12301:8 (Diff revision 5) > +This patch implement several things: > +1. Implement a new constructor for nsStyleImageRequest to receive a existed > +ImageValue from the caller. > +2. Implement Gecko_ImageValue_Create to allow stylo create a gecko::ImageValue > +object. > +3. Implement Gecko_SetXXXXImageValue to allow stylo pass a created ImageValue back to s/pass/to pass/ ::: layout/style/ServoBindings.h:296 (Diff revision 5) > +void Gecko_SetListStyleImageValue(nsStyleList* style_struct, > + mozilla::css::ImageValue* aImageValue); Should this be "Gecko_SetListStyleImageImageValue", for consistency with the other functions? ::: layout/style/ServoBindings.cpp:1220 (Diff revision 5) > +} > + > void > Gecko_SetUrlImageValue(nsStyleImage* aImage, ServoBundledURI aURI) > { > + MOZ_ASSERT(aImage); Nit: indented one space too much. ::: layout/style/ServoBindings.cpp:1255 (Diff revision 5) > { > aStyleUI->mCursorImages.Clear(); > aStyleUI->mCursorImages.SetLength(aLen); > } > > +void Gecko_SetCursorImageValue(nsCursorImage* aCursor, Nit: newline after "void". ::: layout/style/ServoBindings.cpp:1261 (Diff revision 5) > + mozilla::css::ImageValue* aImageValue) > +{ > + MOZ_ASSERT(aCursor && aImageValue); > + > + aCursor->mImage = > + CreateStyleImageRequest(nsStyleImageRequest::Mode::Track, aImageValue); Should this be Mode::Discard? ::: layout/style/ServoBindings.cpp:1349 (Diff revision 5) > + mozilla::css::ImageValue* aImageValue) > +{ > + MOZ_ASSERT(aList && aImageValue); > + > + aList->mListStyleImage = > + CreateStyleImageRequest(nsStyleImageRequest::Mode::Track, aImageValue); Should this be Mode(0)? ::: layout/style/nsStyleStruct.h:350 (Diff revision 5) > + nsStyleImageRequest( > + Mode aModeFlags, > + mozilla::css::ImageValue* aImageValue); Please add a comment above here saying that this constructor can be called from any thread. Actually you can just copy the comment from the constructor just above.
Attachment #8862811 - Flags: review?(cam) → review+
Comment on attachment 8864431 [details] Bug 1310885 - Part 4. (gecko) Allow ImageValue::Intialize be called more then once, but only LoadImage in the first call. https://reviewboard.mozilla.org/r/136120/#review139836 ::: layout/style/nsCSSValue.h:233 (Diff revision 2) > > nsRefPtrHashtable<nsPtrHashKey<nsIDocument>, imgRequestProxy> mRequests; > > private: > -#ifdef DEBUG > bool mInitialized = false; Let's rename this to mLoadedImage or something like that.
Attachment #8864431 - Flags: review?(cam) → review+
Comment on attachment 8865140 [details] Bug 1310885 - Part 5. (stylo) Let SpecifiedUrl be able to carry ImageValue. https://reviewboard.mozilla.org/r/136802/#review139838 ::: servo/components/style/gecko/url.rs:15 (Diff revision 1) > use parser::ParserContext; > use std::borrow::Cow; > use std::fmt::{self, Write}; > use style_traits::ToCss; > use stylearc::Arc; > +use gecko_bindings::structs::root::mozilla::css::ImageValue; Move this line up to its sorted position. (Servo's |./mach test-tidy| will probably complain about this otherwise.) ::: servo/components/style/gecko/url.rs:29 (Diff revision 1) > serialization: Arc<String>, > > /// The URL extra data. > pub extra_data: RefPtr<URLExtraData>, > + > + /// Carry ImageValue if any. Please expand this comment to explain why we carry around this ImageValue object on the specified value. ::: servo/components/style/gecko/url.rs:85 (Diff revision 1) > mURLStringLength: len as u32, > mExtraData: self.extra_data.get(), > } > } > + > + /// Build and carry an image value when request. "when requested" or "on request"
Attachment #8865140 - Flags: review?(cam) → review+
Comment on attachment 8865141 [details] Bug 1310885 - Part 6. (stylo) Pass LayerImage's image_value to gecko https://reviewboard.mozilla.org/r/136804/#review139840 ::: servo/components/style/gecko/conversions.rs:146 (Diff revision 1) > - Gecko_SetUrlImageValue(self, url.for_ffi()); > + if let Some(ref image_value) = url.image_value { > + Gecko_SetLayerImageImageValue(self, image_value.get()); > + } Should we always have a non-None image_value here? If so, just do: Gecko_SetLayerImageImageValue(self, url.image_value.unwrap().get()); ::: servo/components/style/gecko/conversions.rs:161 (Diff revision 1) > - Gecko_SetUrlImageValue(self, image_rect.url.for_ffi()); > + if let Some(ref image_value) = image_rect.url.image_value { > + Gecko_SetLayerImageImageValue(self, image_value.get()); > + } And here. ::: servo/components/style/values/specified/image.rs:56 (Diff revision 1) > > impl Image { > #[allow(missing_docs)] > pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<Image, ()> { > - if let Ok(url) = input.try(|input| SpecifiedUrl::parse(context, input)) { > + if let Ok(mut url) = input.try(|input| SpecifiedUrl::parse(context, input)) { > + url.build_image_value(); I think this will cause Servo (not stylo) to not compile. You can wrap this statement with: if cfg!(feature = "gecko") { ... } ::: servo/components/style/values/specified/image.rs:63 (Diff revision 1) > } > if let Ok(gradient) = input.try(|input| Gradient::parse_function(context, input)) { > return Ok(Image::Gradient(gradient)); > } > - if let Ok(image_rect) = input.try(|input| ImageRect::parse(context, input)) { > + if let Ok(mut image_rect) = input.try(|input| ImageRect::parse(context, input)) { > + image_rect.url.build_image_value(); And here.
Attachment #8865141 - Flags: review?(cam) → review+
Comment on attachment 8865142 [details] Bug 1310885 - Part 7. (stylo) Pass content's image_value to gecko. https://reviewboard.mozilla.org/r/136806/#review139842 ::: servo/components/style/properties/gecko.mako.rs:4159 (Diff revision 1) > - unsafe { bindings::Gecko_SetContentDataImage(&mut self.gecko.mContents[i], url.for_ffi()) } > + if let Some(image_value) = url.image_value { > + unsafe { > + bindings::Gecko_SetContentDataImageValue(&mut self.gecko.mContents[i], image_value.get()) > + } > + } Just unwrap().
Attachment #8865142 - Flags: review?(cam) → review+
Comment on attachment 8865143 [details] Bug 1310885 - Part 8. (stylo) Pass cursor's image_value to gecko. https://reviewboard.mozilla.org/r/136808/#review139844 ::: servo/components/style/properties/gecko.mako.rs:3994 (Diff revision 1) > + if let Some(ref image_value) = image.url.image_value { > - unsafe { > + unsafe { > - Gecko_SetCursorImage(&mut self.gecko.mCursorImages[i], image.url.for_ffi()); > + Gecko_SetCursorImageValue(&mut self.gecko.mCursorImages[i], image_value.get()); > + } > } Just unwrap().
Attachment #8865143 - Flags: review?(cam) → review+
Comment on attachment 8865144 [details] Bug 1310885 - Part 9. (stylo) Pass style list's image_value to gecko. https://reviewboard.mozilla.org/r/136810/#review139846 ::: servo/components/style/properties/gecko.mako.rs:2967 (Diff revision 1) > + if let Some(ref image_value) = url.image_value { > - unsafe { > + unsafe { > - Gecko_SetListStyleImage(&mut self.gecko, > - url.for_ffi()); > + Gecko_SetListStyleImageValue(&mut self.gecko, > + image_value.get()); > + } > } Just unwrap(). ::: servo/components/style/properties/longhand/list.mako.rs:77 (Diff revision 1) > + if let Either::First(ref mut url) = value { > + url.build_image_value(); > + } Wrap this in % if product == "gecko": .... % endif
Attachment #8865144 - Flags: review?(cam) → review+
Comment on attachment 8863642 [details] Bug 1310885 - Part 10. (gecko) Always release ImageValue on the main thread. https://reviewboard.mozilla.org/r/135444/#review139848 Currently the nsStyleImageRequest destructor will dispatch the StyleImageRequestCleanupTask to the main thread to release the ImageValue object, if we called Resolve() on the nsStyleImageRequest at some point. So I'm curious when you ran into this assertion -- was it for nsStyleImageRequest objects that weren't resolved? I'm a bit concerned that it's possible to release the specified value (e.g. by removing the style sheet from the document) before the nsStyleImageRequest destructor runs. I was going to suggest just dispatching the StyleImageRequestCleanupTask to the main thread even when the object is not resolved, so that we always release the ImageValue on the main thread. But we use the mDocGroup to dispatch the runnable, and mDocGroup only gets set in Resolve(). If this is a rare case, maybe we can just forget about Quantum DOM labelling for unresolved nsStyleImageRequest objects, and dispatch the runnable with NS_DispatchToMainThread if mDocGroup is null. But I'd like to understand why we are hitting the assertion that you want to remove. ::: commit-message-6cc5e:11 (Diff revision 4) > + "thread to release ImageValues!"); > + > +The reason why I think we can just remove this assertion is: > +There is another assertion at [1] which assert on the same thing and is more > +correctly. At ~StyleImageRequestCleanupTask, it will call ImageValue::Release > +but calling this funciton does not mean ImageValue will be _deleted_, unless s/funciton /function/ ::: commit-message-6cc5e:12 (Diff revision 4) > +the refcount of this ImageValue is 1 before calling. Since ImageValue is still > +been hold by XXX::SpecifiedValue, which is destructed on the main thread, we "has still been held" or "is still being held"
Attachment #8863642 - Flags: review?(cam)
Comment on attachment 8862815 [details] Bug 1310885 - Part 11. (gecko) Clean up unused things. https://reviewboard.mozilla.org/r/134738/#review139852
Attachment #8862815 - Flags: review?(cam) → review+
Comment on attachment 8863642 [details] Bug 1310885 - Part 10. (gecko) Always release ImageValue on the main thread. https://reviewboard.mozilla.org/r/135444/#review139848 I saw this assertion failure on /test_compute_data_with_start_struct.html only https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f963af3e5b56a4f647fd57ba736bbffb41362d0&selectedJob=95287164 The reason of hitting this assertion is right the same with what you said.
Comment on attachment 8863642 [details] Bug 1310885 - Part 10. (gecko) Always release ImageValue on the main thread. https://reviewboard.mozilla.org/r/135444/#review139952
Attachment #8863642 - Flags: review?(cam) → review+
Whiteboard: leave-open
Here is the landing plan 1. land part 0 to servo repo(done) 2. land part 1 on inbounds after #1 be merged back to central 3. land part 2 to servo repo after #2 be merged back to central. 4. land part 3/4 to inbound after #3 be merged back to central 5. land part 5/6/7/8/9 to servo repo after #4 be merged back to central. 6. land part 10/11 on inbounds after #5 be merged back to central.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/006659d83f3b Part 1. (gecko) Export RefPtr<ImageValue> from gecko to stylo.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2db290c4b6 Part 3. (gecko) Create a new API to receive cached ImageValue from stylo. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9245a2fbb974 Part 4. (gecko) Allow ImageValue::Intialize be called more then once, but only LoadImage in the first call. r=haycam https://hg.mozilla.org/integration/mozilla-inbound/rev/77352010d8e8 Part 5. (gecko) Always release ImageValue on the main thread. r=heycam
CJ, you'll need to make some changes in js/src/devtools/rootAnalysis/analyzeHeapWrites.js to make the static analysis happy. Going through the three new hazards in https://queue.taskcluster.net/v1/task/QUes8XoVSRS23zLUHHNlaQ/runs/0/artifacts/public/build/heapWriteHazards.txt.gz: Error: AddRef/Release on nsISupports Location: _ZN12nsStyleImage7SetNullEv$void nsStyleImage::SetNull() @ layout/style/nsStyleStruct.cpp#2255 ### SafeArguments: <this> Stack Trace: Gecko_SetNullImageValue @ layout/style/ServoBindings.cpp#1146 ### SafeArguments: <arg0> https://hg.mozilla.org/integration/mozilla-inbound/file/77352010d8e8a269cf33e7891f4c00f2d73b4541/layout/style/nsStyleStruct.cpp#l2255 It looks like this is complaining about the Release() call on nsStyleImage::mElementId, which is an nsIAtom. That should be safe -- maybe this is not being caught in checkOverridableVirtualCall? sfink, how can we modify checkOverridableVirtualCall to allow a call to nsIAtom::Release without a RefPtr/nsCOMPtr being involved? For now, I guess we can add Gecko_SetNullImageValue to the ignoreContents whitelist, but we should fix that once we work out how to whitelist the nsIAtom::Release call. Error: Field write nsStyleImage:field:2.mImage nsStyleImage.field:2 Location: _ZN12nsStyleImage15SetImageRequestE16already_AddRefedI19nsStyleImageRequestE$void nsStyleImage::SetImageRequest(struct already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp#2274 ### SafeArguments: aImage Stack Trace: Gecko_SetLayerImageImageValue @ layout/style/ServoBindings.cpp#1201 For this we just need to add Gecko_SetLayerImageImageValue to the treatAsSafeArgument whitelist (just like we have Gecko_SetUrlImageValue in there). Error: Field write class RefPtr<nsStyleImageRequest>.mRawPtr Location: _ZN6RefPtrI19nsStyleImageRequestE22assign_assuming_AddRefEPS0_$void RefPtr<T>::assign_assuming_AddRef(T*) [with T = nsStyleImageRequest] @ obj-analyzed/dist/include/mozilla/RefPtr.h#63 Stack Trace: _ZN6RefPtrI19nsStyleImageRequestEaSIS0_EERS1_O16already_AddRefedIT_E$RefPtr<T>& RefPtr<T>::operator=(already_AddRefed<U>&&) [with I = nsStyleImageRequest; T = nsStyleImageRequest] @ obj-analyzed/dist/include/mozilla/RefPtr.h#209 ### SafeArguments: aCursor Gecko_SetCursorImageValue @ layout/style/ServoBindings.cpp#1248 Same here, we need Gecko_SetCursorImageValue in the treatAsSafeArgument whitelist, just like Gecko_SetCursorImage is. When you make these changes, make sure to do a try run that includes |-p linux64-haz| so you can check that the hazard analysis is happy.
Flags: needinfo?(sphink)
Flags: needinfo?(cku)
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/137f674ff568 Part 3. (gecko) Create a new API to receive cached ImageValue from stylo. https://hg.mozilla.org/integration/mozilla-inbound/rev/15121b2b88a8 Part 4. (gecko) Allow ImageValue::Intialize be called more then once, but only LoadImage in the first call. https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0d38c12ddd Part 5. (gecko) Always release ImageValue on the main thread.
Thanks, heycam. Done, and verified on try that includes linux64-haz https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff3f155218cb543fb687fd874b34fc81f7a9793
Flags: needinfo?(cku)
keep a note: clean up unused functions in both binging.rs and analyzeHeapWrites.js
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/39137a07f063 Part 11. (gecko) Clean up unused things. r=heycam
Looks like it's all landed -- thanks for fixing this, CJ! It was one of the followups to the image support I was worried about.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
I believe all of the hazard issues have been resolved at this point. (I could be wrong.)
Flags: needinfo?(sphink)
(In reply to Steve Fink [:sfink] [:s:] from comment #87) > I believe all of the hazard issues have been resolved at this point. (I > could be wrong.) Yes, we're now at zero hazards [1]. Thanks a lot Steve! We really couldn't have confidently shipped stylo without this. [1] https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/taskcluster/scripts/builder/hazard-analysis.sh#167
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: