Closed
Bug 1372433
Opened 7 years ago
Closed 7 years ago
Label PContent::Msg_NotifyVisited
Categories
(Core :: DOM: Content Processes, enhancement)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: billm, Assigned: nika)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee: nobody → michael
Assignee | ||
Comment 2•7 years ago
|
||
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
Attachment #8879298 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 3•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
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.
Attachment #8879298 -
Flags: review?(wmccloskey) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b5ca196c54
Label the PContent::Msg_NotifyVisited runnable, r=billm
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb550def80d
Part 2: Fix build bustage on a CLOSED TREE, a=bustage
Pushed by michael@thelayzells.com:
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
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
Flags: needinfo?(michael)
Updated•7 years ago
|
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
Assignee | ||
Comment 10•7 years ago
|
||
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
Attachment #8895445 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•7 years ago
|
Attachment #8879298 -
Attachment is obsolete: true
Reporter | ||
Comment 11•7 years ago
|
||
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.
Attachment #8895445 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Flags: needinfo?(michael)
Assignee | ||
Comment 13•7 years ago
|
||
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
Attachment #8895986 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•7 years ago
|
Attachment #8895445 -
Attachment is obsolete: true
Reporter | ||
Comment 14•7 years ago
|
||
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?
Attachment #8895986 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 15•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8895986 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6740f28449d
Label the PContent::Msg_NotifyVisited runnable, r=billm
Comment 17•7 years ago
|
||
bugherder |
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
•