1342873 - Label runnables in layout/tables/

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

All calls to NS_DispatchTo(Main|Current)Thread under layout/tables/.

http://searchfox.org/mozilla-central/search?q=NS_DispatchTo(Main%7CCurrent)Thread&case=false&regexp=true&path=layout%2Ftables

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Review commit: https://reviewboard.mozilla.org/r/118420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118420/

Comment 2

[Jeremy Chen ([e-mail])]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118420/diff/1-2/

Comment 3

[Jeremy Chen ([e-mail])]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118420/diff/2-3/

Comment 4

[Jeremy Chen ([e-mail])]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Try with this patch looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11341b4992a9459db9dec7129dac67efcaf5155

I'm not sure if this code path would sometimes go through off-main thread, so I sent a try to check this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be0279806a4c2fdfc4ef4365f5f1e87ce957b09

Looks like there's no crash on try. However, I'm not familiar with these codes, I can't be sure if we could remove the "NS_IsMainThread" branch condition in this patch.


Hi Cameron, could you take a look at this and give me some feedback? Thanks.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

https://reviewboard.mozilla.org/r/118420/#review120402

f+, the actual Dispatch call looks right, though the rest of the code around it could be simplified a bit.

::: layout/tables/nsTableFrame.cpp:5008
(Diff revision 3)
>      // XXX In principle this should only be necessary for border style changes
>      // However the bc painting code tries to maximize the drawn border segments
>      // so it stores in the cellmap where a new border segment starts and this
>      // introduces a unwanted cellmap data dependence on color
>      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> +    if (NS_IsMainThread()) {

We don't need to check if we're on the main thread here.  We should always be on the main thread here (looks like BCRecalcNeeded is just called from DidSetStyleContext, which is done on the main thread).  So maybe the use of NS_DispatchToCurrentThread is a little misleading, since it sounds like it needs to handle us running on any thread.  (I'm not sure if there is some advantage to using NS_DispatchToCurrentThread when we know we're on the main thread, but I did notice that there are lots of uses of it that could be replaced with NS_DispatchToMainThread with no change in behaviour.)

As far as I know, it is safe to call Dispatch even if we are dispatching to a non-main thread; it's just not necessary.  So for simplicity, if we did need to handle different threads here, we should still just use a single Dispatch call, rather than switching to decide whether to call NS_DispatchToCurrentThread instead.

::: layout/tables/nsTableFrame.cpp:5009
(Diff revision 3)
>      // However the bc painting code tries to maximize the drawn border segments
>      // so it stores in the cellmap where a new border segment starts and this
>      // introduces a unwanted cellmap data dependence on color
>      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> +    if (NS_IsMainThread()) {
> +      nsCOMPtr<nsIDocument> doc = this->GetContent()->OwnerDoc();

No need for "this->".

Also, OwnerDoc() never returns nulls, so no need to null check it.

Comment 6

[Jeremy Chen ([e-mail])]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8845224 [details]
> Bug 1342873 - wip.
> 
> https://reviewboard.mozilla.org/r/118420/#review120402
> 
> f+, the actual Dispatch call looks right, though the rest of the code around
> it could be simplified a bit.
> 
> ::: layout/tables/nsTableFrame.cpp:5008
> (Diff revision 3)
> >      // XXX In principle this should only be necessary for border style changes
> >      // However the bc painting code tries to maximize the drawn border segments
> >      // so it stores in the cellmap where a new border segment starts and this
> >      // introduces a unwanted cellmap data dependence on color
> >      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> > +    if (NS_IsMainThread()) {
> 
> We don't need to check if we're on the main thread here.  We should always
> be on the main thread here (looks like BCRecalcNeeded is just called from
> DidSetStyleContext, which is done on the main thread).  So maybe the use of
> NS_DispatchToCurrentThread is a little misleading, since it sounds like it
> needs to handle us running on any thread.  (I'm not sure if there is some
> advantage to using NS_DispatchToCurrentThread when we know we're on the main
> thread, but I did notice that there are lots of uses of it that could be
> replaced with NS_DispatchToMainThread with no change in behaviour.)

Yes, for main thread cases, DispatchToMainThread and DispatchToCurrentThread should behave the same. However, I'm not sure if replacing DispatchToCurrentThread with DispatchToMainThread on main thread cases would be more clear or not, since DispatchToMainThread seems implicitly represents a potential cross-threads event looping.

> As far as I know, it is safe to call Dispatch even if we are dispatching to
> a non-main thread; it's just not necessary.  So for simplicity, if we did
> need to handle different threads here, we should still just use a single
> Dispatch call, rather than switching to decide whether to call
> NS_DispatchToCurrentThread instead.

Okay, sounds fair enough.

> ::: layout/tables/nsTableFrame.cpp:5009
> (Diff revision 3)
> >      // However the bc painting code tries to maximize the drawn border segments
> >      // so it stores in the cellmap where a new border segment starts and this
> >      // introduces a unwanted cellmap data dependence on color
> >      nsCOMPtr<nsIRunnable> evt = new nsDelayedCalcBCBorders(this);
> > +    if (NS_IsMainThread()) {
> > +      nsCOMPtr<nsIDocument> doc = this->GetContent()->OwnerDoc();
> 
> No need for "this->".
> 
> Also, OwnerDoc() never returns nulls, so no need to null check it.

Lesson learned!!!! Thank you.

Comment 7

[Jeremy Chen ([e-mail])]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118420/diff/3-4/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8845224 [details]
Bug 1342873 - label runnables of nsDelayedCalcBCBorders for nsTableFrame.

https://reviewboard.mozilla.org/r/118420/#review120446

Looks good, thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6932ea4168a2
label runnables of nsDelayedCalcBCBorders for nsTableFrame. r=heycam

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6932ea4168a2