Closed Bug 1338446 Opened 7 years ago Closed 7 years ago

label runnables in layout/style/

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: TYLin)

References

Details

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

User Story

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

Attachments

(6 files)

Bug 1338446 Part 1 - Label dispatching ShortTermURISpecCache by using SystemGroup.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338446 Part 2 - Label FontFaceSet::CheckLoadingFinishedAfterDelay.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338446 Part 3 - Label SheetLoadData in Loader::PostLoadEvent.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338446 Part 4 - Label StyleImageRequestCleanupTask.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338446 Part 5 - Label runnable in Gecko_DropElementSnapshot by using SystemGroup.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338446 Part 6 - Label LoadTimer in nsFontFaceLoader::StartedLoading.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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.
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
I think those are all correct.  As far as I know DOM event dispatches should all be Other.
No longer blocks: Labeling
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
Assignee: nobody → tlin
Status: NEW → ASSIGNED
User Story: (updated)
User Story: (updated)
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
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+
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+
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-
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
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+
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 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.
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
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
(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
That nsStringBuffer leak might be bug 1347065.
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+
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-
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+
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
(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 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+
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
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: