1372433 - Label PContent::Msg_NotifyVisited

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This runnable is very common, so we need to assign it a TabGroup somehow. I'm not really sure how to do that. It might make sense to use the SystemGroup and then to dispatch individual SetLinkState runnables to each separate tab. I don't think we care too much about latency here.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Overholt suggested I should help out with labeling. I can take this one.

I think that doing your approach of dispatching individual SetLinkState runnables to each TabGroup sounds like the best one, so that's what I'll do at least at first.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Label the PContent::Msg_NotifyVisited runnable

I don't bother to label the runnables in the parent process being fired by
VisitedQuery, as we are not planning to perform scheduling in the parent process
if I remember correctly. It would be possible to label those runnables as well.

MozReview-Commit-ID: EosNOu62fEV

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I should write a note.

I had to do an inefficient iteration over the entire list for each document which is listening to the link because I needed to support unregistering links between receiving the message from the parent process, and the actual links being updated.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8879298 [details] [diff] [review]
Label the PContent::Msg_NotifyVisited runnable

Review of attachment 8879298 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/History.cpp
@@ +1992,5 @@
> +
> +void
> +History::NotifyVisitedForDocument(nsIURI* aURI, nsIDocument* aDocument)
> +{
> +  nsAutoScriptBlocker scriptBlocker;

Is the script blocker to prevent the iterator from being invalidated? Whatever the reason, it deserves a comment.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b5ca196c54
Label the PContent::Msg_NotifyVisited runnable, r=billm

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb550def80d
Part 2: Fix build bustage on a CLOSED TREE, a=bustage

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3abe2b5f8b24
Part 3: Explicitly check SystemGroup is initialized before getting its event target on a CLOSED TREE, a=bustage

Comment 8

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

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5e736fdcac99fe1c6df3e155719b42c207f05488 for assertions failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111783735&repo=mozilla-inbound

Comment 9

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

There were also non-stylo assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111834498&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=111835784&repo=mozilla-inbound

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Label the PContent::Msg_NotifyVisited runnable

I don't bother to label the runnables in the parent process being fired by
VisitedQuery, as we are not planning to perform scheduling in the parent process
if I remember correctly. It would be possible to label those runnables as well.

This is an updated version which avoids ever synchronously setting the Link as
visited, always making sure to dispatch a seperate runnable to do it. I think
that in the previous patch it was possible that a link would be registered
during styling, which could then cause a restyle during styling, which caused
the assertion failures.

MozReview-Commit-ID: EosNOu62fEV

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8895445 [details] [diff] [review]
Label the PContent::Msg_NotifyVisited runnable

Review of attachment 8895445 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +3572,5 @@
>  {
> +  if ((aMsg.type() == PJavaScript::Msg_DropTemporaryStrongReferences__ID
> +       || aMsg.type() == PJavaScript::Msg_DropObject__ID
> +       || aMsg.type() == PContent::Msg_NotifyVisited__ID)
> +      && SystemGroup::Initialized()) {

I don't think you need this check since bug 1384631 landed.

::: toolkit/components/places/History.cpp
@@ +1962,5 @@
> +  Element* element = aLink->GetElement();
> +  nsCOMPtr<nsIDocument> doc = element ? element->OwnerDoc() : nullptr;
> +
> +  // If someone's already dispatched a runnable for this document, don't.
> +  if (aKey->mPendingDocRunnables.Contains(doc)) {

This doesn't seem right to me. Suppose that we notify for URL U and we've already notified document A but not document B. Now a new link to U is added to A. As far as I can tell, that link will start in the unvisited state and it will never be notified.

I'm also a little worried about ABA problems here since we don't have a strong reference. In practice this doesn't seem very likely, but I'd rather avoid such things if we can.

This document table seems like an optimization that we might not need. Could we remove it? We're never going to double-notify individual links since they get removed from the array when we set their state.

@@ +2019,5 @@
> +  }
> +
> +  // If we don't have any links left, we can remove the array.
> +  if (key->array.IsEmpty()) {
> +    mObservers.RemoveEntry(key);

Since we're not removing any links here, why do we need this?

@@ +2028,5 @@
> +
> +void
> +History::NotifyVisitedForDocument(nsIURI* aURI, nsIDocument* aDocument)
> +{
> +  nsAutoScriptBlocker scriptBlocker;

I still would like a comment about the purpose of the script blocker.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bill McCloskey (:billm) from comment #11)
> I still would like a comment about the purpose of the script blocker.

The primary reason why it is present there is that it was around in the original version of the patch. Looking into it a bit more it seems like SetLinkState could potentially trigger script execution, and we want to be sure that we don't accidentally mutate our observers etc. as we're iterating over them.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Label the PContent::Msg_NotifyVisited runnable

I don't bother to label the runnables in the parent process being fired by
VisitedQuery, as we are not planning to perform scheduling in the parent process
if I remember correctly. It would be possible to label those runnables as well.

MozReview-Commit-ID: EosNOu62fEV

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8895986 [details] [diff] [review]
Label the PContent::Msg_NotifyVisited runnable

Review of attachment 8895986 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: toolkit/components/places/History.cpp
@@ +2039,5 @@
> +      // NOTE: Theoretically GetElement should never return nullptr, but it does
> +      // in GTests because they use a mock_Link which returns null from this
> +      // method.
> +      Element* element = link->GetElement();
> +      nsIDocument* doc = element ? element->OwnerDoc() : nullptr;

Did you mean to use GetLinkDocument here?

@@ +2701,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  // If this link has already been visited, we cannot synchronously mark
> +  // ourselves as visited, so instead we fire a runnable into our docgroupw

Typo in the last character.

::: toolkit/components/places/History.h
@@ +217,5 @@
>      {
>        return array.ShallowSizeOfExcludingThis(aMallocSizeOf);
>      }
>      ObserverArray array;
> +    bool mSeen;

I don't see anything initializing this to false. Can you just add |= false| here? Also, please write a short comment describing what this is for. Also, mVisited might be a better name?

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Label the PContent::Msg_NotifyVisited runnable

I don't bother to label the runnables in the parent process being fired by
VisitedQuery, as we are not planning to perform scheduling in the parent process
if I remember correctly. It would be possible to label those runnables as well.

MozReview-Commit-ID: EosNOu62fEV

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6740f28449d
Label the PContent::Msg_NotifyVisited runnable, r=billm

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f6740f28449d