Closed
Bug 1377158
Opened 7 years ago
Closed 7 years ago
stylo: Enable stylo for SVG-as-an-image.
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: kuoe0.tw, Assigned: kuoe0.tw)
References
Details
Attachments
(5 files, 6 obsolete files)
I found SVG-as-an-image is not parsed by stylo. In [1], we pass aContainer with nullptr to create the document. The document without a container will not be parsed by stylo [2]. So, I tend to set the backend type after the document created[3] and before the document loading[4]. [1]: https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/image/SVGDocumentWrapper.cpp#344-349 [2]: http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/dom/base/nsDocument.cpp#13173 [3]: https://dxr.mozilla.org/mozilla-central/rev/9af23c413a1f8d337b19b4f8450e241e91b71136/layout/build/nsContentDLF.cpp#366 [4]: https://dxr.mozilla.org/mozilla-central/rev/9af23c413a1f8d337b19b4f8450e241e91b71136/layout/build/nsContentDLF.cpp#377
Assignee | ||
Updated•7 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•7 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) |
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•7 years ago
|
Attachment #8886051 -
Attachment is obsolete: true
Assignee | ||
Updated•7 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•7 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)
Updated•7 years ago
|
Priority: -- → P1
Comment 20•7 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•7 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.
Comment 22•7 years ago
|
||
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•7 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•7 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•7 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•7 years ago
|
||
Oh, so aLoadingDocument is the document which contains the images would be loaded. Is it right?
Comment 27•7 years ago
|
||
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•7 years ago
|
Attachment #8886046 -
Flags: review?(cam)
Attachment #8886049 -
Flags: review?(cam)
Attachment #8887813 -
Flags: review?(cam)
Assignee | ||
Comment 34•7 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) |
Comment 42•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8889336 -
Flags: review?(cam)
Comment 60•7 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.
Comment 61•7 years ago
|
||
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•7 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) |
Comment 65•7 years ago
|
||
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•7 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+
Assignee | ||
Comment 67•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dad74e18a806b0963e2a582e56f7676a15913a3
Comment 68•7 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
Comment 69•7 years ago
|
||
backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=117484105&repo=autoland
Flags: needinfo?(kuoe0)
Comment 70•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a769d1e11513 Backed out changeset 2fbce7c06627 https://hg.mozilla.org/integration/autoland/rev/cc6128e68428 Backed out changeset 30b7a45177bf https://hg.mozilla.org/integration/autoland/rev/01e4f85ad7f6 Backed out changeset 1310d6d8b5c0 https://hg.mozilla.org/integration/autoland/rev/a013e95212fc Backed out changeset b05327c3c55f https://hg.mozilla.org/integration/autoland/rev/1bcb249ab430 Backed out changeset 61637891db5f https://hg.mozilla.org/integration/autoland/rev/941e995a4a30 Backed out changeset 9a76090acf6e https://hg.mozilla.org/integration/autoland/rev/1f122ddfeedd Backed out changeset 7c3ddf34fc42 for stylo failures
Comment 71•7 years ago
|
||
(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•7 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•7 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•7 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
Comment 75•7 years ago
|
||
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)
Comment 76•7 years ago
|
||
My feeling was that to minimize risk we should probably avoid using stylo SVG images used throughout chrome windows for now.
Comment 77•7 years ago
|
||
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•7 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•7 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 80•7 years ago
|
||
Hmmm... They[1] still happen. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f35c1e233d742d0f487b8a59101728d59875bc4
Assignee | ||
Comment 81•7 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
Comment 82•7 years ago
|
||
What about my idea in comment 75?
Assignee | ||
Comment 83•7 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)
Comment 84•7 years ago
|
||
(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•7 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)
Comment 86•7 years ago
|
||
(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)
Updated•7 years ago
|
Priority: P1 → --
Assignee | ||
Comment 87•7 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) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8887813 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892782 -
Flags: review?(cam)
Attachment #8892783 -
Flags: review?(cam)
Comment 99•7 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•7 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+
Updated•7 years ago
|
Priority: -- → P1
Comment 101•7 years ago
|
||
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.
Comment 102•7 years ago
|
||
(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•7 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•7 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!
Comment 105•7 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. 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•7 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)
Comment 107•7 years ago
|
||
(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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8886047 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8886048 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8886049 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8889336 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 119•7 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•7 years ago
|
Attachment #8886046 -
Flags: review?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 126•7 years ago
|
||
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 127•7 years ago
|
||
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+
Comment 128•7 years ago
|
||
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•7 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) |
Updated•7 years ago
|
Flags: needinfo?(cam)
Attachment #8886046 -
Flags: review?(cam) → review+
Updated•7 years ago
|
Attachment #8886050 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 136•7 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)
Comment 137•7 years ago
|
||
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)
Comment 138•7 years ago
|
||
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)
Comment 139•7 years ago
|
||
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)
Comment 140•7 years ago
|
||
(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)
Comment 141•7 years ago
|
||
OK, can do.
Comment 142•7 years ago
|
||
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•7 years ago
|
||
I'll apply your patch and make sure everything is fine before landing this bug. Thx!
Assignee | ||
Comment 144•7 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
Comment 145•7 years ago
|
||
Bug 1388319 has landed on autoland last night.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 148•7 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
Comment 149•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/32414fdc3dcf https://hg.mozilla.org/mozilla-central/rev/c8d944dfc0a5 https://hg.mozilla.org/mozilla-central/rev/075472d87ab2 https://hg.mozilla.org/mozilla-central/rev/8e991e3b80f6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•