Cameron McCormack (:heycam) Reporter
	Description • 8 years ago

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).

	u459114 Assignee
	Comment 10 • 7 years ago

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)

	u459114 Assignee
	Comment 11 • 7 years ago

(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.

	Cameron McCormack (:heycam) Reporter
	Comment 46 • 7 years ago
mozreview-review

Comment on attachment 8863640 [details]
Bug 1310885 - Part 0. (gecko) Export mozilla::css::ImageValue to stylo.

https://reviewboard.mozilla.org/r/135440/#review139826

	Cameron McCormack (:heycam) Reporter
	Comment 47 • 7 years ago
mozreview-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

	Cameron McCormack (:heycam) Reporter
	Comment 48 • 7 years ago
mozreview-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

	Cameron McCormack (:heycam) Reporter
	Comment 49 • 7 years ago
mozreview-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.

	Cameron McCormack (:heycam) Reporter
	Comment 50 • 7 years ago
mozreview-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.

	Cameron McCormack (:heycam) Reporter
	Comment 51 • 7 years ago
mozreview-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"

	Cameron McCormack (:heycam) Reporter
	Comment 52 • 7 years ago
mozreview-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.

	Cameron McCormack (:heycam) Reporter
	Comment 53 • 7 years ago
mozreview-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().

	Cameron McCormack (:heycam) Reporter
	Comment 54 • 7 years ago
mozreview-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().

	Cameron McCormack (:heycam) Reporter
	Comment 55 • 7 years ago
mozreview-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

	Cameron McCormack (:heycam) Reporter
	Comment 56 • 7 years ago
mozreview-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"

	Cameron McCormack (:heycam) Reporter
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8862815 [details]
Bug 1310885 - Part 11. (gecko) Clean up unused things.

https://reviewboard.mozilla.org/r/134738/#review139852

	u459114 Assignee
	Comment 58 • 7 years ago
mozreview-review-reply

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.

	Cameron McCormack (:heycam) Reporter
	Comment 71 • 7 years ago
mozreview-review

Comment on attachment 8863642 [details]
Bug 1310885 - Part 10. (gecko) Always release ImageValue on the main thread.

https://reviewboard.mozilla.org/r/135444/#review139952

	u459114 Assignee
	Comment 72 • 7 years ago

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.

	Pulsebot
	Comment 73 • 7 years ago

Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/006659d83f3b
Part 1. (gecko) Export RefPtr<ImageValue> from gecko to stylo.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 74 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/006659d83f3b

	u459114 Assignee
	Comment 75 • 7 years ago

Part 0: https://github.com/servo/servo/pull/16765
Part 1: https://hg.mozilla.org/mozilla-central/rev/006659d83f3b
Part 2: https://github.com/servo/servo/pull/16789

	Pulsebot
	Comment 76 • 7 years ago

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

	Phil Ringnalda (:philor)
	Comment 77 • 7 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/644548803654 for heap write hazard failures, https://treeherder.mozilla.org/logviewer.html#?job_id=98611891&repo=mozilla-inbound

	Cameron McCormack (:heycam) Reporter
	Comment 78 • 7 years ago

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.

	Pulsebot
	Comment 79 • 7 years ago

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.

	u459114 Assignee
	Comment 80 • 7 years ago

Thanks, heycam.
Done, and verified on try that includes linux64-haz
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff3f155218cb543fb687fd874b34fc81f7a9793

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 81 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/137f674ff568
https://hg.mozilla.org/mozilla-central/rev/15121b2b88a8
https://hg.mozilla.org/mozilla-central/rev/3d0d38c12ddd

	u459114 Assignee
	Comment 82 • 7 years ago

keep a note: clean up unused functions in both binging.rs and analyzeHeapWrites.js

	u459114 Assignee
	Comment 83 • 7 years ago

Part 6~10
https://github.com/servo/servo/pull/16862

	Pulsebot
	Comment 84 • 7 years ago

Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39137a07f063
Part 11. (gecko) Clean up unused things. r=heycam

	Carsten Book [:Tomcat]
	Comment 85 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/39137a07f063

	Cameron McCormack (:heycam) Reporter
	Comment 86 • 7 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 87 • 6 years ago

I believe all of the hazard issues have been resolved at this point. (I could be wrong.)

	Bobby Holley (:bholley)
	Comment 88 • 6 years ago

(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