stylo: Enable stylo for SVG-as-an-image.

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(5 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
1.36 KB, patch
Details | Diff | Splinter Review
Assignee

Updated

2 years ago
Summary: stylo: Pass backend type to CreateDocument() to specitfy the backend type of documents. → stylo: Enable stylo for SVG-as-an-image.
Assignee

Comment 1

2 years ago
Due to the SVG used in chrome document should be parsed by gecko. So, we can't set the backend type to servo for all SVG documents. I think we should set the backend type according to where the `nsStyleImageRequest` object is created from.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8886051 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8886050 - Flags: review?(cam)
Attachment #8886046 - Flags: review?(cam)
Attachment #8886047 - Flags: review?(cam)
Attachment #8886048 - Flags: review?(cam)
Attachment #8886049 - Flags: review?(cam)
Assignee

Comment 19

2 years ago
Because `context-{fill|stroke}-opacity` are not implemented in stylo, it would cause the test cases I added in Bug 1373159 failed. Do you think I should add fail-if(stylo) for these test cases? Or, we can land this bug after Bug 1338764 landed?
Flags: needinfo?(cam)
Assignee

Updated

2 years ago
Blocks: stylo-svg
Priority: -- → P1

Comment 20

2 years ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162390

::: image/imgRequest.cpp:1134
(Diff revision 3)
>    // appropriate image.
>    if (newPartPending) {
>      NewPartResult result = PrepareForNewPart(aRequest, aInStr, aCount, mURI,
>                                               isMultipart, image,
> -                                             progressTracker, mInnerWindowId);
> +                                             progressTracker, mInnerWindowId,
> +                                             IsChrome() ? StyleBackendType::Gecko :

I'm not sure IsChrome() is the exact thing we want to check.  That returns true if the image URL is a chrome:// URL, but there are at least some other chrome-ish URLs like resource:// that are used to load SVG images from chrome documents.  And add-ons can have their own random protocols.

Ideally, we would know whether the requesting document is a content document or not (or perhaps whether it is a stylo-styled document or not) and use that to determine whether the SVG image it loads should use stylo or not.  I remember we discussed choosing based on which nsStyleImageRequest constructor was called, but I guess that only works for images loaded from CSS, and we probably want to control this for HTML <img> elements too.

CSS image loads and HTML <img> loads all end up going through nsContentUtils::LoadImage, I think.  And it looks like it passes through the document that caused the image load to imgLoader::LoadImage.  Can we make the decision at that point, in imgLoader::LoadImage, by taking aLoadingDocument's StyleBackendType and passing it in to imgRequest::Init?

Comment 21

2 years ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches.

https://reviewboard.mozilla.org/r/156838/#review162388

::: image/SVGDocumentWrapper.cpp:351
(Diff revision 3)
> +  RefPtr<nsContentDLF> contentDLF =
> +    static_cast<nsContentDLF*>(docLoaderFactory.get());
> +  NS_ENSURE_TRUE(contentDLF, NS_ERROR_UNEXPECTED);

In theory the nsIDocumentLoaderFactory we get back might be some other object and not nsContentDLF.  (I don't think that's the case for Firefox, but there might be other XUL-based apps that could do this.)

So for now, while we still have to use this indirect XPCOM stuff, I think we should just add an argument to the nsIDocumentLoaderFactory createInstance function, which we can pass down to CreateDocument, so that we don't have to do this cast here.
I'll hold off on the rest of the reviews for comment 20.

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #19)
> Because `context-{fill|stroke}-opacity` are not implemented in stylo, it
> would cause the test cases I added in Bug 1373159 failed. Do you think I
> should add fail-if(stylo) for these test cases? Or, we can land this bug
> after Bug 1338764 landed?

I think it's fine to mark them as fails-if(stylo), if this bug lands first, and then we can remove the annotations in bug 1338764.
Flags: needinfo?(cam)
Assignee

Comment 23

2 years ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162426

::: image/imgRequest.cpp:1134
(Diff revision 3)
>    // appropriate image.
>    if (newPartPending) {
>      NewPartResult result = PrepareForNewPart(aRequest, aInStr, aCount, mURI,
>                                               isMultipart, image,
> -                                             progressTracker, mInnerWindowId);
> +                                             progressTracker, mInnerWindowId,
> +                                             IsChrome() ? StyleBackendType::Gecko :

Did you mean we should call GetStyleBackendType() from aLoadingDocument? Because the SVG document created from SVGDocumentWrapper doesn't have a container, the style backend type would be always Gecko here. [1]

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1256
Assignee

Comment 24

2 years ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches.

https://reviewboard.mozilla.org/r/156838/#review162424

::: image/SVGDocumentWrapper.cpp:351
(Diff revision 3)
> +  RefPtr<nsContentDLF> contentDLF =
> +    static_cast<nsContentDLF*>(docLoaderFactory.get());
> +  NS_ENSURE_TRUE(contentDLF, NS_ERROR_UNEXPECTED);

Okay, I just think it's better to not change the interface. But if it's possible to be other object, I think we should change the interface.

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review162426

> Did you mean we should call GetStyleBackendType() from aLoadingDocument? Because the SVG document created from SVGDocumentWrapper doesn't have a container, the style backend type would be always Gecko here. [1]
> 
> [1]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1256

I think aLoadingDocument should be the document that caused the image load, e.g. the HTML document that has the <img> element or which has elements styled with background-image, etc.  That document should already have decided its StyleBackendType, since we're rendering it.
Assignee

Comment 26

2 years ago
Oh, so aLoadingDocument is the document which contains the images would be loaded. Is it right?
I think so!  At the point that nsContentUtils::LoadImage is called, the referenced SVG document image hasn't been created yet.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8886046 - Flags: review?(cam)
Attachment #8886049 - Flags: review?(cam)
Attachment #8887813 - Flags: review?(cam)
Assignee

Comment 34

2 years ago
Hi Cameron, I haven't modified the issue you mentioned for part 2. Please review the other parts first.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 41

2 years ago
It's ready to review!
Flags: needinfo?(cam)

Comment 42

2 years ago
mozreview-review
Comment on attachment 8886050 [details]
Bug 1377158 - (Part 1) Set style backend to stylo when SVG is used as an image.

https://reviewboard.mozilla.org/r/156846/#review162382

r=me with these comments addressed.

::: docshell/base/nsIDocumentLoaderFactory.idl:27
(Diff revision 5)
>   * The component is a service, so use GetService, not CreateInstance to get it.
>   */
>  
>  [scriptable, uuid(e795239e-9d3c-47c4-b063-9e600fb3b287)]
>  interface nsIDocumentLoaderFactory : nsISupports {
>      nsIContentViewer createInstance(in string aCommand,

I'm not sure if there really are other JS users of this API.  But I think to be complete, we should add some consts here for them to use.  It will make the API look a bit more complete.  So something like:

  const octet STYLE_BACKEND_TYPE_NONE = 0;
  const octet ...

And I know this is not documented at all, but can you add a comment here to say that aStyleBackendType can be used to choose the style backend type for the created document, or STYLE_BACKEND_TYPE_NONE can be used to mean the default will be used.

::: dom/base/nsDocument.cpp:1022
(Diff revision 5)
>  
>    nsCOMPtr<nsIContentViewer> viewer;
>    nsCOMPtr<nsIStreamListener> listener;
>    rv = docLoaderFactory->CreateInstance("external-resource", chan, newLoadGroup,
>                                          type, nullptr, nullptr,
> +                                        0 /* StyleBackendType::None */,

With the XPIDL constants added, you can write:

  nsIDocumentLoaderFactory::STYLE_BACKENDTYPE_NONE,

(Here and in the other functions that call CreateInstance.)

::: layout/build/nsContentDLF.h:13
(Diff revision 5)
>  
>  #include "nsIDocumentLoaderFactory.h"
>  #include "nsMimeTypes.h"
> +#include "mozilla/StyleBackendType.h"
> +
> +using mozilla::StyleBackendType;

Nit: generally we prefer not to add "using" statements to header files, since it can affect other files including this one.  So I would prefer "mozilla::StyleBackendType" to be written out full in the CreateDocument declaration.  But adding a "using namespace mozilla;" is OK in the .cpp file.

::: layout/build/nsContentDLF.cpp:136
(Diff revision 5)
>  
>  NS_IMETHODIMP
>  nsContentDLF::CreateInstance(const char* aCommand,

Just above here, can you add some static assertions that each of the three values of the XPIDL constants match the values of the C++ StyleBackendType enum?

::: layout/build/nsContentDLF.cpp:381
(Diff revision 5)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Set style backend type before document loading. Can't set None as the
> +  // backend type, it would hit an assertion.
> +  if (aStyleBackendType != StyleBackendType::None) {
> +    // To dynamic switch the backend type, check stylo is enabled or not

"dynamically"
Attachment #8886050 - Flags: review?(cam) → review+

Comment 43

2 years ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches.

https://reviewboard.mozilla.org/r/156838/#review164498

::: image/SVGDocumentWrapper.h:43
(Diff revision 5)
>    SVGDocumentWrapper();
> +  explicit SVGDocumentWrapper(StyleBackendType aStyleBackendType);

Just make aStyleBackendType an optional argument of the existing constructor.
Attachment #8886046 - Flags: review?(cam) → review+

Comment 44

2 years ago
mozreview-review
Comment on attachment 8886047 [details]
Bug 1377158 - (Part 3) Add style backend type member in VectorImage to be used to create SVGDocumentWrapper with specified style backend.

https://reviewboard.mozilla.org/r/156840/#review164500
Attachment #8886047 - Flags: review?(cam) → review+

Comment 45

2 years ago
mozreview-review
Comment on attachment 8886048 [details]
Bug 1377158 - (Part 4) Add a new parameter to pass style backend type to create image.

https://reviewboard.mozilla.org/r/156842/#review164502
Attachment #8886048 - Flags: review?(cam) → review+

Comment 46

2 years ago
mozreview-review
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches.

https://reviewboard.mozilla.org/r/156838/#review164504

::: commit-message-6fec4:1
(Diff revision 5)
> +Bug 1377158 - (Part 2) Add style backend type member in SvgDocumentWrapper and create SVG document with specified style backend.

"SVGDocumentWrapper"

Comment 47

2 years ago
mozreview-review
Comment on attachment 8886049 [details]
Bug 1377158 - (Part 5) Decide style backend type according to the loading document.

https://reviewboard.mozilla.org/r/156844/#review164508

::: image/imgLoader.cpp:2240
(Diff revision 5)
>      nsCOMPtr<nsILoadGroup> channelLoadGroup;
>      newChannel->GetLoadGroup(getter_AddRefs(channelLoadGroup));
>      rv = request->Init(aURI, aURI, /* aHadInsecureRedirect = */ false,

Please add a comment in here saying that we look at the loading document's style backend type so that Chrome documents, where stylo is disabled, will use a Gecko backend for any SVG-as-image documents they load.
Attachment #8886049 - Flags: review?(cam) → review+

Comment 48

2 years ago
mozreview-review
Comment on attachment 8887813 [details]
Bug 1377158 - (Part 7) Disable failed test cases.

https://reviewboard.mozilla.org/r/158720/#review164510

Looks great, thanks for fixing this!
Attachment #8887813 - Flags: review?(cam) → review+
Assignee

Comment 49

2 years ago
We expected the test cases of context-*-opacity should be failed after we enabled SVG-as-an-image for stylo. But I noticed the test cases still passed in the mode "styloVsGecko". I think this is caused by the image cache. When an SVG image is loaded, it would be cached. And we'll use it when we reload the page. So, when we turn off the stylo pref and reload the page, it still uses the same image to compare.
Flags: needinfo?(cam)
Assignee

Comment 50

2 years ago
I'll add style backend type to image cache and make stylo and gecko can get different cache.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8889336 - Flags: review?(cam)

Comment 60

2 years ago
mozreview-review
Comment on attachment 8889336 [details]
Bug 1377158 - (Part 6) Add StyleBackend to hash function to cache different SVG images generated from Gecko and Stylo.

https://reviewboard.mozilla.org/r/160410/#review166028

::: image/ImageCacheKey.cpp:20
(Diff revision 2)
>  #include "nsPrintfCString.h"
>  
>  namespace mozilla {
>  
>  using namespace dom;
> +using mozilla::StyleBackendType;

This shouldn't be needed, since we're inside the "namespace mozilla {}" block.

::: image/ImageCacheKey.cpp:66
(Diff revision 2)
>  
>    if (URISchemeIs(mURI, "blob")) {
>      mBlobSerial = BlobSerial(mURI);
>    }
>  
> -  mHash = ComputeHash(mURI, mBlobSerial, mOriginAttributes, mControlledDocument);
> +  mHash = ComputeHash(mURI, mBlobSerial, mOriginAttributes, mControlledDocument, 

Nit: trailing white space.
This final patch looks fine, but I notice that ImageCacheKey already stores the nsIDocument pointer, and compares it in operator==, so I'm a bit confused about why you were getting those test failures (and thus why these changes, which just affect the hash value, fix it).  Do you know?
Flags: needinfo?(kuoe0)
Assignee

Comment 62

2 years ago
I fount we use mControlledDocument to compute hash value. And I noticed the mControlledDocument always is `null`. In [1], we return null for non-controlled documents. Maybe the documents we loaded are all non-controlled documents. But actually I don't know what are controlled documents and non-controlled documents.

[1]: https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/image/ImageCacheKey.cpp#148-164
Flags: needinfo?(kuoe0)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Oh, I see.  Controlled documents seem to be some Service Worker related thing I don't understand either.  So aDocument is non-null in the ImageCacheKey constructor, but we usually set mControlledDocument to null (and so we wouldn't effectively be comparing the StyleBackendType previously).

Comment 66

2 years ago
mozreview-review
Comment on attachment 8889336 [details]
Bug 1377158 - (Part 6) Add StyleBackend to hash function to cache different SVG images generated from Gecko and Stylo.

https://reviewboard.mozilla.org/r/160410/#review166112

Thanks for checking the document thing.
Attachment #8889336 - Flags: review?(cam) → review+

Comment 68

2 years ago
Pushed by tokuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c3ddf34fc42
(Part 1) Make nsIDocumentLoaderFactory::createInstance() accept style backend type. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9a76090acf6e
(Part 2) Add style backend type member in SVGDocumentWrapper and create SVG document with specified style backend. r=heycam
https://hg.mozilla.org/integration/autoland/rev/61637891db5f
(Part 3) Add style backend type member in VectorImage to be used to create SVGDocumentWrapper with specified style backend. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b05327c3c55f
(Part 4) Add a new parameter to pass style backend type to create image. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1310d6d8b5c0
(Part 5) Decide style backend type according to the loading document. r=heycam
https://hg.mozilla.org/integration/autoland/rev/30b7a45177bf
(Part 6) Add StyleBackend to hash function to cache different SVG images generated from Gecko and Stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2fbce7c06627
(Part 7) Disable failed test cases. r=heycam
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #67)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1dad74e18a806b0963e2a582e56f7676a15913a3

It seems there are still several failures in your try push here, which eventually hits autoland with your landing.

Given this try push, I have some suggestions.

Firstly, please don't push with known failures (or known high frequent intermittents introduced from your patch as I suggested in bug 1355746 comment 99), because that would just waste everyone's time without anything productive.

I can see that there is potential intermittent bug listed for all those failures, so you might think those are just intermittents and will go away when landing. However, there are multiple tasks fail at the same position, which is a signal that your patch may be guilty to the failure. In that case, you should retrigger the failing tasks to be sure whether it is really an intermittent. It is also better to retrigger if the bug description doesn't seem to exactly match the failure message.

This should be handled especially carefully when the patch may substantially change how code runs (like in this case, it enables a new area for the code to work).

Secondly, it doesn't really seem to me you need an all-all try push for this bug. A try push with build on all platforms but test linux64 (including stylo) only should generally be enough.

Thanks for working on this.
Assignee

Comment 72

2 years ago
Sorry, I didn't notice the failures are different to the intermittent failures. I'll look at the failures and figure it out why they happened.
Flags: needinfo?(kuoe0)
Assignee

Comment 73

2 years ago
I tested locally today. I found the part 5 patch would increase the intermittent rate of [1], even be failed every time in debug build. About the reftest failures of "Rs7" and "Rs15", they are still failed even I removed all my patches. That's weird.

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=117853765&repo=try&lineNumber=17595
Assignee

Comment 74

2 years ago
BTW, the error msg from [1] is always "load failed with unknown reason". Currently, I have no idea about why it failed.

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=117853765&repo=try&lineNumber=17595
A question: can we just check whether IsBeingUsedAsImage is true inside nsIDocument::UpdateStyleBackendType() before checking whether there is document container?

I suppose SVG inside chrome document isn't really much different than those in content document, so probably there is no need for distinguishing between them.

That way, the patch set for this bug can probably be much simpler.

WDYT, Tommy?
Flags: needinfo?(kuoe0)
My feeling was that to minimize risk we should probably avoid using stylo SVG images used throughout chrome windows for now.
My feeling is that if we make code unnecessarily complicated... it is harder to go back.

SVG images should just be SVG images. They shouldn't include much Mozilla-specific things, I suppose.

A simple search in our codebase seems to confirm my opinion: https://dxr.mozilla.org/mozilla-central/search?q=%22-moz-%22%20ext%3Asvg&case=true&=mozilla-central

From that result, the only -moz-prefixed things used in chrome SVG are:
* -moz-windows-default-theme and -moz-os-version media features
* -moz-MenuBarText, -moz-field, -moz-dialog colors
and it seems to me all of them have been exposed to web content.

Also SVG images in chrome is probably also a good test set for our SVG support in general. If we can successfully handle SVG images in chrome, we have less possibility to see bug in web content.
Assignee

Comment 78

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #75)
> A question: can we just check whether IsBeingUsedAsImage is true inside
> nsIDocument::UpdateStyleBackendType() before checking whether there is
> document container?
> 
> I suppose SVG inside chrome document isn't really much different than those
> in content document, so probably there is no need for distinguishing between
> them.
> 
> That way, the patch set for this bug can probably be much simpler.
> 
> WDYT, Tommy?

I'll investigate that and discuss that with Cameron tomorrow.

BTW, I rebased my patches to m-c today and ran a try [1] again. That's weird, the failures we met before are already gone.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7df614a5ec2f7beb0a48d2070edce5efbac85f9
Flags: needinfo?(kuoe0)
Assignee

Comment 79

2 years ago
Oh, I'm wrong... I rebased with m-c yesterday, but my patches already are backed out. So, it is not merged successfully. I'll run a try again.
Assignee

Comment 81

2 years ago
After some investigation, I found the reason caused the crash test[1] failed. The assertion we used here[2] takes too much time when there are a lots of elements. That's why this test only happens in debug builds. I'll keep looking at it and try to reduce the cost.

And about the other test case failures (Rs7 and Rs15) with tiny RGB value difference, I tend to increase the fuzzy range for them. They only happen when comparing stylo and gecko. I don't think they are a big issue for now, and I'll file another bug for them.

[1]: https://dxr.mozilla.org/mozilla-central/source/image/test/crashtests/694165-1.xhtml
[2]: https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/layout/base/nsStyleChangeList.cpp#47-50
What about my idea in comment 75?
Assignee

Comment 83

2 years ago
Hi Xidorn, I discussed that with Cameron last week. He thought we should use the original way. We think we should use Gecko for the document parsed by Gecko and use Stylo for the one parsed by Stylo. And because the patches are almost ready.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cam)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #83)
> Hi Xidorn, I discussed that with Cameron last week. He thought we should use
> the original way. We think we should use Gecko for the document parsed by
> Gecko and use Stylo for the one parsed by Stylo.

All documents are parsed by Gecko. Stylo only parses stylesheets, and neither Gecko nor Stylo should start parsing stylesheets before we decide the backend in nsIDocument::UpdateStyleBackendType().

Also, SVG-as-images are independent from their parent document (ideally nothing should be able to access content inside SVG-as-image), so I don't think whether an SVG-as-image document is a content document really matters a lot.

> And because the patches are almost ready.

Well... I don't really agree with this. As I mentioned before, it is always easier to complicate code than to simplify code. Once the code becomes more complicated, it is pretty hard to have it back when we no longer need that complexity. So if we can avoid adding such complexity from the very beginning, we should avoid it.
Flags: needinfo?(xidorn+moz)
Assignee

Comment 85

2 years ago
Hi Bobby, could you explain this assertion[2] to me? This assertion take a long time when this crash test[1] is running.

I think it is used to prevent to insert two or more reconstructFrame hint for the same content. Is that right? 

[1]: https://dxr.mozilla.org/mozilla-central/source/image/test/crashtests/694165-1.xhtml
[2]: https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/layout/base/nsStyleChangeList.cpp#47-50
Flags: needinfo?(bobbyholley)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #85)
> Hi Bobby, could you explain this assertion[2] to me? This assertion take a
> long time when this crash test[1] is running.
> 
> I think it is used to prevent to insert two or more reconstructFrame hint
> for the same content. Is that right? 

That's right. The basic idea is to assert against the case that Gecko handles below.

Gecko will also walk the entire array, but only in the case where |aHint| is a reconstruct hint. Stylo walks the array unconditionally, to find the cases where we append a reconstruct and then _later_ append a non-reconstruct hint.

However, I think that condition isn't a correctness issue (since ProcessRestyledFrames will handle that case via mDestroyedFrames) - so we can go ahead and weaken the assertion by hoisting the reconstruct check to operate on aHint outside of the loop. That should solve the timeout issue, which may speed up other debug tests as well.

Please add a comment while you're at it to explain why we're doing that, and why the weakened check is ok.
Flags: needinfo?(bobbyholley)
Priority: P1 → --
Assignee

Comment 87

2 years ago
I think Xidorn already finished Bug 1338764, so I'll remove the `fails-if` for the test cases of context-*-opacity. I'll run a try after Xidorn's patches landed to check that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8887813 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8892782 - Flags: review?(cam)
Attachment #8892783 - Flags: review?(cam)

Comment 99

2 years ago
mozreview-review
Comment on attachment 8892782 [details]
Bug 1377158 - (Part 3) Update the expectation of test cases.

https://reviewboard.mozilla.org/r/163772/#review169506
Attachment #8892782 - Flags: review+

Comment 100

2 years ago
mozreview-review
Comment on attachment 8892783 [details]
Bug 1377158 - (Part 4) Reduce the assertion cost to check reconstruction frame hint.

https://reviewboard.mozilla.org/r/163774/#review169508

r=me with the comment fixed.

::: layout/base/nsStyleChangeList.cpp:48
(Diff revision 2)
> +      // XXXkuoe0 It cannot catch the case that we append an nsStyleChangeData
> +      // without nsChangeHint_ReconstructFrame of content A when there is
> +      // already an nsStyleChangeData with nsChangeHint_ReconstructFrame of
> +      // content A.

Nit: No need for the XXX here.

Let's make the comment say:

// Note: Because we check whether |aHint| is a reconstruct above (which is
// necessary to avoid debug test timeouts on certain crashtests), this check
// will not find bugs where we add a non-reconstruct hint for an element after
// adding a reconstruct. This is ok though, since ProcessRestyledFrames will
// handle that case via mDestroyedFrames.
Attachment #8892783 - Flags: review+
Priority: -- → P1
FWIW, what I proposed is something like this, to replace part 1 ~ 6.

I confirmed that it works, via checking that the test marked fails-if in layout/reftests/svg/as-image/reftest.list does fail with this patch applied.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #101)
> FWIW, what I proposed is something like this, to replace part 1 ~ 6.

Probably we need part 6 either way. So part 1 ~ 5.
Assignee

Comment 103

2 years ago
I also noticed that this intermittent (Bug 1384211) seems always fails when this patches applied.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2171bec494d73876b5cefa37aad12956f09db8f&selectedJob=120326325
Assignee

Comment 104

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #101)
> Created attachment 8893161 [details] [diff] [review]
> alternative patch
> 
> FWIW, what I proposed is something like this, to replace part 1 ~ 6.
> 
> I confirmed that it works, via checking that the test marked fails-if in
> layout/reftests/svg/as-image/reftest.list does fail with this patch applied.

That's a really simpler patch, and I prefer to use this approach. I'll apply it and run a try again to check that. Thanks!
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #101)
> Created attachment 8893161 [details] [diff] [review]
> alternative patch
> 
> FWIW, what I proposed is something like this, to replace part 1 ~ 6.
> 
> I confirmed that it works, via checking that the test marked fails-if in
> layout/reftests/svg/as-image/reftest.list does fail with this patch applied.

Try push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0800083bd0fbcd2a66c0ab83cee58ac002432aeb
(since this patch doesn't touch non-stylo code, I didn't run non-stylo tasks.)

One unexpected failure on context-fill-02.html and one unexpected crash on svg-glyph-basic.svg.
Assignee

Comment 106

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #105)
> Try push for this patch:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0800083bd0fbcd2a66c0ab83cee58ac002432aeb
> (since this patch doesn't touch non-stylo code, I didn't run non-stylo
> tasks.)
> 
> One unexpected failure on context-fill-02.html and one unexpected crash on
> svg-glyph-basic.svg.

I also found the leak check of Rs15 always failed in both your try push[1] and mine[2]. They are all related to svg-glyphs. Should we ignore it for now? I think we could expect svg-glyphs is not ready on stylo.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0800083bd0fbcd2a66c0ab83cee58ac002432aeb
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebc622b20fb96e93384779291b2675d56e8909c
Flags: needinfo?(xidorn+moz)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #106)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #105)
> > Try push for this patch:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=0800083bd0fbcd2a66c0ab83cee58ac002432aeb
> > (since this patch doesn't touch non-stylo code, I didn't run non-stylo
> > tasks.)
> > 
> > One unexpected failure on context-fill-02.html and one unexpected crash on
> > svg-glyph-basic.svg.
> 
> I also found the leak check of Rs15 always failed in both your try push[1]
> and mine[2]. They are all related to svg-glyphs. Should we ignore it for
> now? I think we could expect svg-glyphs is not ready on stylo.

That's not a leak check failure, that's a crash for
> thread '<unnamed>' panicked at 'assertion failed: !self.is_device_dirty', /home/worker/workspace/build/src/servo/components/style/stylist.rs:1179

I guess it is probably fine to skip that test for now, since this assertion looks more like an sanity check. But we definitely should figure out what's happening there. Please file a bug against bug 1289964.
Flags: needinfo?(xidorn+moz)

Comment 108

2 years ago
mozreview-review
Comment on attachment 8892782 [details]
Bug 1377158 - (Part 3) Update the expectation of test cases.

https://reviewboard.mozilla.org/r/163772/#review169642
Attachment #8892782 - Flags: review?(cam) → review+

Comment 109

2 years ago
mozreview-review
Comment on attachment 8892783 [details]
Bug 1377158 - (Part 4) Reduce the assertion cost to check reconstruction frame hint.

https://reviewboard.mozilla.org/r/163774/#review169646

::: layout/base/nsStyleChangeList.cpp:55
(Diff revision 2)
> +                   "This should be the only reconstruct frame hint of this \
> +                   content.");

I think the message is slightly wrong.  Maybe:

  Should not append a non-ReconstructFrame hint after
  appending a ReconstructFrame hint for the same
  content.
Attachment #8892783 - Flags: review?(cam) → review+
Assignee

Comment 110

2 years ago
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #103)
> I also noticed that this intermittent (Bug 1384211) seems always fails when
> this patches applied.
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a2171bec494d73876b5cefa37aad12956f09db8f&selectedJob=1
> 20326325

I found the part 6 caused this intermittent rate very high, and it always failed on debug build. I suspect the reason is the timeout[1] is too shrot. I'll increase the timeout and run a try. BTW, I can't reproduce it locally.

[1]: https://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/browser/components/places/tests/browser/browser_views_iconsupdate.js#67
Assignee

Comment 111

2 years ago
Cameron, do you have any idea with the test failure I mentioned in comment 110. I tried to increase the timeout, and the fail rate seems decreased. But it's still very high.

try with only Xidorn's patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c030b9ea07fe12ab5ca087d940f77e6f67da781
try with part 6 patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebc622b20fb96e93384779291b2675d56e8909c
try with increasing the timeout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a4a09cefd6d96e1cd873e92fabaae0ec2ebbe3
Assignee

Comment 112

2 years ago
I think we just add a new parameter to hash function, and it could not affect the favicon loading. Even it caused we can not get the favicon cache, we can just reload the favicon again. I don't think it would make it failed.
Assignee

Comment 113

2 years ago
After discussed with Xidorn, I think the part 6 is wrong. With applying Xidorn's patch, we won't decide the style backend for images according to the doc which loading them. So, we can't put the style backend of the loading doc to ImageCacheKey, it doesn't represent the style backend of the images.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8886047 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8886048 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8886049 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8889336 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Assignee

Comment 119

2 years ago
After update the patch which change the hash function, the test case bc2 is passed[1]. I'll test all patches again and check the part 2 is correct on try.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dff32a9db952bc8a576bee2d87c5361016bea84
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8886046 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8886050 [details]
Bug 1377158 - (Part 1) Set style backend to stylo when SVG is used as an image.

This part pretty much needs a re-review.
Attachment #8886050 - Flags: review+ → review?(cam)
Comment on attachment 8886046 [details]
Bug 1377158 - (Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches.

This is mostly based on my idea, so it is better asking someone else to have a look.
Attachment #8886046 - Flags: review?(xidorn+moz)
Attachment #8886046 - Flags: review?(cam)
Attachment #8886046 - Flags: review+
To make it clear, the problem of the old part 6 is not just what stated in comment 113. It seems to be mainly because of that operator== wasn't updated to comparing the backend. That means, there could exist two image cache key, which are identical according to operator==, but generate different hash value. This violates the basic invariant of hash table.

I have no idea how this violation could lead to that intermittent, but according to Tommy's try runs, it seems that this was indeed the reason.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 131

2 years ago
The part 2 (original part 6) seems correct. I ran a try without context-*-opacity implementation, and the test cases failed[1] as expected. And it passed[2] with context-*-opacity implementation.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5b2acb001cd345078c758223a252895f610c1d&selectedJob=121376780
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=010e4785f7b6072fd2703823ef7253b786c2fc2f&selectedJob=121379512
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Flags: needinfo?(cam)
Attachment #8886046 - Flags: review?(cam) → review+
Attachment #8886050 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 136

2 years ago
After disabled some crashes of test cases we found, more crashes happen[1]. Should we disable all test cases of svg-glpyh on debug build for now? Or, just try until there is no more crash?

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed793f3cecb619ec96b1f911dd200e1cceade76&selectedJob=121598839
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cam)
Looking at the log you linked to, all five of those crashes are the same debug assertion.  So probably we can skip those five too, and investigate in the followup bug.
Flags: needinfo?(cam)
Do we use any SVG-in-OpenType in chrome? If yes, we probably should fix those assertions before landing it, otherwise people would not be able to open a debug build of Firefox with stylo enabled. Otherwise, I guess it is fine to skip them for now and investigate in the followup bug.

From the stack, it seems like we don't get a chance to call Stylist::rebuild for SVG-in-OT in some cases before resolving style? Not sure why that could happen.
Flags: needinfo?(xidorn+moz)
Discussed with emilio on IRC, and digging into the code a bit, it seems that:
* `ServoStyleSet` is constructed with `mStylistState` being `NotDirty`. [1]
* `Stylist` is constructed with `is_device_dirty` being `true`. [2]
* We pre-fill StyleSet with some stylesheets in `nsDocument::FillStyleSet` [3] before calling StyleSet::Init.
* When we pre-fill StyleSet, `ServoStyleSet` doesn't change `mStylistState` because `mRawSet` is null. [4]
* When we call `ServoStyleSet::Init`, it hands the prefilled sheets to Servo side, but still thinks everything is clean. [5]

That says, if there is no any stylesheet inserted after `ServoStyleSet::Init` is called, Gecko would think everything is clean, while Servo would think it isn't. This would lead to this assertion.

This may also explain some of other failures show up in reftests of SVG glyphs, especially the context value related ones, because we do have something related in svg.css, [6] so if we don't flush stylist properly, we may end up styling wrong things.

I think the correct fix would be, setting the stylist state to dirty at the end of `ServoStyleSet::Init`, or at least conditionally do so when we do have sheets inserted.

What do you think, heycam?


[1] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/style/ServoStyleSet.cpp#43
[2] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/style/stylist.rs#240
[3] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/dom/base/nsDocument.cpp#2394
[4] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/style/ServoStyleSet.cpp#614-620
[5] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/style/ServoStyleSet.cpp#68-85
[6] https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/svg/svg.css#26-36
Flags: needinfo?(cam)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #139)
> What do you think, heycam?

I think that all makes sense.  Do you want to fix that in a separate, dependent bug?
Flags: needinfo?(cam)
OK, can do.
Depends on: 1388319
I tried locally, and it seems that with my patch in bug 1388319 applied, all tests in layout/reftests/text-svgglyphs pass with stylo. When I reverted my patch, I can see the same assertion here happens.
Assignee

Comment 143

2 years ago
I'll apply your patch and make sure everything is fine before landing this bug. Thx!
Assignee

Comment 144

2 years ago
It looks great on try[1] with the patch in Bug 1388319. I'll land this bug after Bug 1388319 landed.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=375fbaff991a744311dc36e71a144f5107b5a99e
Bug 1388319 has landed on autoland last night.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 148

2 years ago
Pushed by tokuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32414fdc3dcf
(Part 1) Set style backend to stylo when SVG is used as an image. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c8d944dfc0a5
(Part 2) Add the info of StyloEnabled() to hash function to make reftests of styloVsGecko get the correct caches. r=heycam
https://hg.mozilla.org/integration/autoland/rev/075472d87ab2
(Part 3) Update the expectation of test cases. r=bholley,heycam
https://hg.mozilla.org/integration/autoland/rev/8e991e3b80f6
(Part 4) Reduce the assertion cost to check reconstruction frame hint. r=bholley,heycam
Duplicate of this bug: 1371187
You need to log in before you can comment on or make changes to this bug.