If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

label runnables in layout/style/

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: heycam, Assigned: TYLin)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

User Story

See https://wiki.mozilla.org/Quantum/DOM for the story.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

7 months ago
There are only five calls to NS_DispatchTo(Main|Current)Thread under layout/style/, and I think they all can use the Other task group.

* In ErrorReporter::~ErrorReporter, to dispatch a runnable to clear a static cache.  This can use SystemGroup.

* In FontFaceSet::OnFontFaceStatusChanged, to dispatch a runnable to do some work after a reflow.  This can use mDocument->Dispatch().  (Not too sure if mDocument needs to be null checked there.)

* In Loader::PostLoadEvent, to do some cleanup, notify observers, and unblock document load.  If the Loader has an mDocument I guess we should call Dispatch() on that, but we might need to check users of document-less Loaders to see whether there really is an appropriate document to dispatch the runnable against, or whether the SystemGroup is fine.

* In nsStyleImageRequest::~nsStyleImageRequest, to do some cleanup work (involving  imagelib) on the main thread.  An nsStyleImageRequest should be associated with a specific document (it lives in style structs), but we don't have easy access to the document here.  It might not matter and we can use the SystemGroup.

* In Gecko_DropElementSnapshot, to destroy a ServoElementData object on the main thread during Servo restyle traversal.  This also shouldn't be visible to content so probably can use SystemGroup.
(Assignee)

Comment 1

7 months ago
Do we need to do something to those calls using AsyncEventDispatcher?  AsyncEventDispatcher::PostDOMEvent() uses "other" task group [1].

[1] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/dom/events/AsyncEventDispatcher.cpp#71,78
(Reporter)

Comment 2

7 months ago
I think those are all correct.  As far as I know DOM event dispatches should all be Other.
Blocks: 1339343
No longer blocks: 1321812
(Assignee)

Comment 3

7 months ago
We might need to consider calls related to TimerCallback.

http://searchfox.org/mozilla-central/search?q=TimerCallback&case=false&regexp=false&path=layout%2Fstyle
Please be informed that the use case of nsExpirationTracker has to be labeled as explained in bug 1345387 comment 0.
http://searchfox.org/mozilla-central/source/layout/style/RuleProcessorCache.h#70
Depends on: 1345464
(Assignee)

Updated

7 months ago
Assignee: nobody → tlin
Status: NEW → ASSIGNED
(Assignee)

Updated

7 months ago
User Story: (updated)
(Assignee)

Updated

7 months ago
User Story: (updated)
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 11

6 months ago
Upload all the patches on my hand. The ExpirationTracker (comment 4) depends on bug 1345464 and can probably fix in a followup bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9646a0505a12726326a114b92ca4c41bee0e9e6
(Reporter)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8847438 [details]
Bug 1338446 Part 1 - Label dispatching ShortTermURISpecCache by using SystemGroup.

https://reviewboard.mozilla.org/r/120400/#review122382
Attachment #8847438 - Flags: review?(cam) → review+
(Reporter)

Comment 13

6 months ago
mozreview-review
Comment on attachment 8847439 [details]
Bug 1338446 Part 2 - Label FontFaceSet::CheckLoadingFinishedAfterDelay.

https://reviewboard.mozilla.org/r/120402/#review122386
Attachment #8847439 - Flags: review?(cam) → review+
(Reporter)

Comment 14

6 months ago
mozreview-review
Comment on attachment 8847440 [details]
Bug 1338446 Part 3 - Label SheetLoadData in Loader::PostLoadEvent.

https://reviewboard.mozilla.org/r/120404/#review122394

::: layout/style/Loader.cpp:2485
(Diff revision 1)
> +    rv = SystemGroup::Dispatch("SheetLoadData", TaskCategory::Other,
> +                               runnable.forget());

I don't think it's correct to always use the SystemGroup for Loader objects that have no mDocument.  It's probably not a correctness issue (I don't think there is other code that depends on the runnable being run before any of the document's other runnables), but it seems like many Loader objects that are created temporarily are used for style sheet loads for a specific document, and so we should ensure that these runnables are prioritised appropriately (over SystemGroup ones, which I believe are deprioritised).

See: http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3css6LoaderC1ENS_16StyleBackendTypeE&redirect=false

For example these two calls seem closely related to a specific document:

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/base/nsDocument.cpp#4416

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsDocumentViewer.cpp#2298

and this one isn't:

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsStyleSheetService.cpp#221

but I'm not sure we get through to posting the runnable in this case; worth checking.
Attachment #8847440 - Flags: review?(cam) → review-
(Reporter)

Comment 15

6 months ago
mozreview-review
Comment on attachment 8847441 [details]
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.

https://reviewboard.mozilla.org/r/120406/#review122438

::: layout/style/nsStyleStruct.cpp:1993
(Diff revision 1)
> -      NS_DispatchToMainThread(task.forget());
> +      SystemGroup::Dispatch("StyleImageRequestCleanupTask",
> +                            TaskCategory::Other, task.forget());

I am unsure whether delaying this runnable, potentially past the existing of the document, is OK.  I pushed a change to try that unconditionally delays doing the cleanup by 5s to see what happens:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e6ae5b7094d0c3af0d92c40626456c4972621
(Reporter)

Comment 16

6 months ago
mozreview-review
Comment on attachment 8847442 [details]
Bug 1338446 Part 5 - Label runnable in Gecko_DropElementSnapshot by using SystemGroup.

https://reviewboard.mozilla.org/r/120408/#review122440
Attachment #8847442 - Flags: review?(cam) → review+
(Reporter)

Comment 17

6 months ago
mozreview-review
Comment on attachment 8847443 [details]
Bug 1338446 Part 6 - Label LoadTimer in nsFontFaceLoader::StartedLoading.

https://reviewboard.mozilla.org/r/120410/#review122446

::: layout/style/nsFontFaceLoader.cpp:49
(Diff revision 1)
>                                     FontFaceSet* aFontFaceSet,
> -                                   nsIChannel* aChannel)
> +                                   nsIChannel* aChannel,
> +                                   nsIDocument* aDocument)

Since we are always created with a FontFaceSet (we can assert it's non-null in here), and the timer creation happens before we null out mFontFaceSet, I think we can just call mFontFaceSet->GetDocument() (well, add that accessor and call it) instead of storing the document pointer on the nsFontFaceLoader.  WDYT?
Blocks: 1347815
No longer blocks: 1347815
ExpirationTracker in RuleProcessorCache.h would be handled in bug 1347815.
Depends on: 1347815
No longer depends on: 1345464
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

6 months ago
mozreview-review-reply
Comment on attachment 8847441 [details]
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.

https://reviewboard.mozilla.org/r/120406/#review122438

> I am unsure whether delaying this runnable, potentially past the existing of the document, is OK.  I pushed a change to try that unconditionally delays doing the cleanup by 5s to see what happens:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e6ae5b7094d0c3af0d92c40626456c4972621

This is the non-main thread clean up task. I use the new approach in the new patchset, though it becames more complex.
(Assignee)

Comment 25

6 months ago
mozreview-review-reply
Comment on attachment 8847440 [details]
Bug 1338446 Part 3 - Label SheetLoadData in Loader::PostLoadEvent.

https://reviewboard.mozilla.org/r/120404/#review122394

> I don't think it's correct to always use the SystemGroup for Loader objects that have no mDocument.  It's probably not a correctness issue (I don't think there is other code that depends on the runnable being run before any of the document's other runnables), but it seems like many Loader objects that are created temporarily are used for style sheet loads for a specific document, and so we should ensure that these runnables are prioritised appropriately (over SystemGroup ones, which I believe are deprioritised).
> 
> See: http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3css6LoaderC1ENS_16StyleBackendTypeE&redirect=false
> 
> For example these two calls seem closely related to a specific document:
> 
> http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/base/nsDocument.cpp#4416
> 
> http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsDocumentViewer.cpp#2298
> 
> and this one isn't:
> 
> http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/base/nsStyleSheetService.cpp#221
> 
> but I'm not sure we get through to posting the runnable in this case; worth checking.

For those two calls closely related to document, I pass the document's DocGroup into the Loader's constructor, and use it to dispatch events later. 

For the record, it's tempting to cache the DocGroup with a valid document, and use it to dispatch as well. But this turns out to cause test failures on many xul document. I do not dig deeper to find the root cause. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f1df2971b1ba240efa389b5437014a43508025
(Assignee)

Comment 26

6 months ago
Latest try result. I cannot tell whether linux64-stylo Rs8 orange is related to Part 4 or not...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b613eee513328ab98fa31b251a1ef8300835f6f9
(Assignee)

Comment 27

6 months ago
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #26)
> Latest try result. I cannot tell whether linux64-stylo Rs8 orange is related to Part 4 or not...

Another try result after rebasing. linux64-stylo Rs8 orange is gone.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3429284958436bca5dd4e1822b0acadd916ed8
(Reporter)

Comment 28

6 months ago
That nsStringBuffer leak might be bug 1347065.
(Reporter)

Comment 29

6 months ago
mozreview-review
Comment on attachment 8847440 [details]
Bug 1338446 Part 3 - Label SheetLoadData in Loader::PostLoadEvent.

https://reviewboard.mozilla.org/r/120404/#review123698

r=me with this.

::: layout/style/Loader.h:195
(Diff revision 2)
>  
>  class Loader final {
>    typedef mozilla::net::ReferrerPolicy ReferrerPolicy;
>  
>  public:
> -  explicit Loader(StyleBackendType aType);
> +  Loader(StyleBackendType aType, mozilla::dom::DocGroup* aDocGroup);

Please add a comment above here explaining what the aDocGroup argument is used for (and how it can be null in some cases).

::: layout/style/Loader.h:578
(Diff revision 2)
> +  // For dispatching events via DocGroup::Dispatch().
> +  mozilla::dom::DocGroup* MOZ_NON_OWNING_REF mDocGroup = nullptr;

I'm not convinced it's safe to make this a non-owning reference.  For Loaders that do have a valid mDocument, we know that the document owns the Loader, and will ensure that it is nulled out if the document is going away.  For cases where we are using mDocGroup, I don't think we're guaranteed that the DocGroup will outlive the Loader.

So I think we should make this a RefPtr.  (It doesn't look like this will cause a cycle, since DocGroup doesn't hold strong references to the documents in it, so we shouldn't need to traverse/unlink to this pointer in Loader's cycle collection macros.)
Attachment #8847440 - Flags: review?(cam) → review+
(Reporter)

Comment 30

6 months ago
mozreview-review
Comment on attachment 8847441 [details]
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.

https://reviewboard.mozilla.org/r/120406/#review123700

Looking good, but one more round.

::: layout/style/nsStyleStruct.h:391
(Diff revision 2)
> +  // Cache nsPresContext parameter passing into Resolve() for the clean up
> +  // task in the destructor.
> +  nsPresContext* mPresContext = nullptr;

I believe that it is safe to use a raw pointer here, since we should only use mPresContext if we are destroying the nsStyleImageRequest from a style worker thread, and when we call Resolve, it should be the case that any subsequent style worker threads for the document will run while the pres context exists.  But I am not 100% sure.  I think I'd be happier we change this to be similar to the other patch, and store a RefPtr<DocGroup> instead.

::: layout/style/nsStyleStruct.cpp:1915
(Diff revision 2)
> -    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(NS_IsMainThread() || mImageValue->mRequests.Count() == 0,
> +               "Run() should run on main thread, or on non-main threads when "
> +               "mImageValue->mRequests is empty!");

The Run method and the destructor do different bits of work, so it might be helpful to make the assertions match what work we're trying to ensure doesn't happen on the style worker threads.

In Run, we just need to ensure that if mRequestProxy is non-null, that we are on the main thread.

::: layout/style/nsStyleStruct.cpp:1940
(Diff revision 2)
> +    MOZ_ASSERT(NS_IsMainThread() || mImageValue->mRequests.Count() == 0,
> +               "Destructor should run on main thread, or on non-main threads "
> +               "when mImageValue->mRequests is empty!");

And in here is where it's important that mImageValue doesn't have any mRequests, and thus is safe to destroy OMT.

Also, if mRequestProxy or mImageTracker are non-null, then we must also be on the main thread, since those objects don't have thread-safe recounting.

::: layout/style/nsStyleStruct.cpp:1997
(Diff revision 2)
>      RefPtr<StyleImageRequestCleanupTask> task =
>          new StyleImageRequestCleanupTask(mModeFlags,
>                                           mRequestProxy.forget(),
>                                           mImageValue.forget(),
>                                           mImageTracker.forget());
> -    if (NS_IsMainThread()) {
> +    if (NS_IsMainThread() || !mPresContext) {

I think using !IsResolved() rather than !mPresContext (which should be equivalent) is a better description of what we are checking for.

::: layout/style/nsStyleStruct.cpp:1999
(Diff revision 2)
> +      MOZ_ASSERT(NS_IsMainThread() || (!mPresContext && !IsResolved()),
> +                 "We forgot to cache mPresContext in Resolve()?");

An assertion ensuring the |IsResolved() == mDocGroup| would be better in the else branch, I think, since that is where we want to use it.
Attachment #8847441 - Flags: review?(cam) → review-
(Reporter)

Comment 31

6 months ago
mozreview-review
Comment on attachment 8847443 [details]
Bug 1338446 Part 6 - Label LoadTimer in nsFontFaceLoader::StartedLoading.

https://reviewboard.mozilla.org/r/120410/#review123704
Attachment #8847443 - Flags: review?(cam) → review+
(Assignee)

Comment 32

6 months ago
mozreview-review-reply
Comment on attachment 8847441 [details]
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.

https://reviewboard.mozilla.org/r/120406/#review123700

> And in here is where it's important that mImageValue doesn't have any mRequests, and thus is safe to destroy OMT.
> 
> Also, if mRequestProxy or mImageTracker are non-null, then we must also be on the main thread, since those objects don't have thread-safe recounting.

ImageTracker implements NS_INLINE_DECL_THREADSAFE_REFCOUNTING in [1]. Perhaps it's sufficient to assert mRequestProxy is non-null and we are on main thread.

[1] http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/dom/base/ImageTracker.h#40
(Reporter)

Comment 33

6 months ago
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #32)
> ImageTracker implements NS_INLINE_DECL_THREADSAFE_REFCOUNTING in [1].
> Perhaps it's sufficient to assert mRequestProxy is non-null and we are on
> main thread.

ImageTracker's destructor does some work that must be done on the main thread (calling into imgIRequest methods), so while it's safe for the refcount to be adjusted, it's not safe if that would cause the object to be deleted.  So I think we want to ensure that we release it on the main thread.
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 40

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=887491ab2319f11b6ad9a27e16db7079c875be18
(Reporter)

Comment 41

6 months ago
mozreview-review
Comment on attachment 8847441 [details]
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.

https://reviewboard.mozilla.org/r/120406/#review123876

Looks good, thanks!
Attachment #8847441 - Flags: review?(cam) → review+

Comment 42

6 months ago
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0165092dfacd
Part 1 - Label dispatching ShortTermURISpecCache by using SystemGroup. r=heycam
https://hg.mozilla.org/integration/autoland/rev/040783ed88b2
Part 2 - Label FontFaceSet::CheckLoadingFinishedAfterDelay. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c86e4966a122
Part 3 - Label SheetLoadData in Loader::PostLoadEvent. r=heycam
https://hg.mozilla.org/integration/autoland/rev/27748ccce77e
Part 4 - Label StyleImageRequestCleanupTask. r=heycam
https://hg.mozilla.org/integration/autoland/rev/21f9aecdc5d7
Part 5 - Label runnable in Gecko_DropElementSnapshot by using SystemGroup. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f018d6eefc41
Part 6 - Label LoadTimer in nsFontFaceLoader::StartedLoading. r=heycam

Comment 43

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0165092dfacd
https://hg.mozilla.org/mozilla-central/rev/040783ed88b2
https://hg.mozilla.org/mozilla-central/rev/c86e4966a122
https://hg.mozilla.org/mozilla-central/rev/27748ccce77e
https://hg.mozilla.org/mozilla-central/rev/21f9aecdc5d7
https://hg.mozilla.org/mozilla-central/rev/f018d6eefc41
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.