Bevis, would you be able to look over my patches to ensure I'm using the groups as expected?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab05e710b36b4c2e592fe5573f71639c0d07190

Ugh, they exported with -U4 instead of -U8, my bad.

Comment on attachment 8862016 [details] [diff] [review] Part 1. Add event target selection for ProgressTracker main thread labelling., v2 Review of attachment 8862016 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to review again after the comments below are addressed, thanks! ::: image/ProgressTracker.cpp @@ +480,5 @@ > + > + RefPtr<dom::DocGroup> docGroup = Move(aDocGroup); > + RefPtr<dom::TabGroup> tabGroup; > + if (docGroup) { > + tabGroup = docGroup->GetTabGroup(); It is not unnecessary to identify if TabGroup is valid. The GetTabGroup() is infallible according to the implementation so as DocGroup::EventTargetFor(). In addition, docGroup::EventTargetFor() is always the same to tabGroup->EventTargetFor(). The TabGroup is only useful for the some corner case while a tab is created before a valid document is specified. If the DocGroup event target is the only thing we want, we could simply the EventTargetState to Default and DocGroup. In addition, if SystemGroup will be one of your use case to replace the "Default" state according to the caller in the future, it will be better to replace the signature of this method to AddObserver(..., nsIEventTarget *aEventTarget) and have XxxGroup::EventTargetFor() provided from caller instead. Furthermore, can we expose Set/GetEventTarget() method in IProgressObserver. Then, we don't really need this EventTargetState anymore. How do you think? @@ +599,5 @@ > + // state when the last observer is removed, so that we select the most > + // appropriate event target when a new observer is added. > + if (empty && mEventTargetState != EventTargetState::Default) { > + mDocGroup = nullptr; > + mTabGroup = nullptr; We don't need EventTargetState if we could get/set EventTarget in IProgressObserver. ::: image/ProgressTracker.h @@ +22,5 @@ > > namespace mozilla { > +namespace dom { > +class DocGroup; > +class TabGroup; Maybe the nsIEventTarget is the only thing we need to expose here if EventTargetFor is the only use case from DocGroup/TabGroup. @@ +189,5 @@ > // with its loading progress. Weak pointers. > + // > + // Add an observer and update the event target based on its DocGroup (if any). > + void AddObserver(IProgressObserver* aObserver, > + RefPtr<dom::DocGroup>&& aDocGroup); ditto. It will be better to define this method as AddObserver(..., nsIEventTarget *aEventTarget). @@ +212,5 @@ > friend class AsyncNotifyRunnable; > friend class AsyncNotifyCurrentStateRunnable; > friend class ImageFactory; > > + enum class EventTargetState : uint8_t { We don't need EventTargetState if we could get/set EventTarget in IProgressObserver. @@ +250,5 @@ > > // Whether this is a progress tracker for a multipart image. > bool mIsMultipart; > + > + EventTargetState mEventTargetState; ditto

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9) > ditto. > It will be better to define this method as AddObserver(..., nsIEventTarget > *aEventTarget). I think we don't this new AddObserver method if we define Set/GetEventTarget in IProgressObserver.

Comment on attachment 8862019 [details] [diff] [review] Part 3. Change DOM and imagelib plumbing to make use of new ProgressTracker labelling., v2 Review of attachment 8862019 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/imgRequest.cpp @@ +208,5 @@ > } > } > > void > +imgRequest::AddProxy(imgRequestProxy* proxy, Same as suggestion in patch part1. imgRequestProxy is a subclass of IProgressObserver. we don't need these new parameters if Set/GetEventTarget() is available in IProgressObserver.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3273ddfb5792a08e925bf8b7f3ac213e8923f32

Comment on attachment 8866507 [details] [diff] [review] Part 3. imgRequestProxy should use an event target derived from the loading document., v2 Review of attachment 8866507 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/imgRequestProxy.cpp @@ +280,5 @@ > + docGroup = aLoadingDocument->GetDocGroup(); > + } > + > + if (docGroup) { > + mEventTarget = docGroup->EventTargetFor(mozilla::TaskCategory::Other); nsIDocument implements DispatcherTrait, so we could simply this as if (aLoadingDocument) { mEventTarget = aLoadingDocument->EventTargetFor(mozilla::TaskCategory::Other); } else if (...) { ... }

Linux appears clean, try on other platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c50a502ff827776276df5728633a6a70caea36d(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #20) > Comment on attachment 8866507 [details] [diff] [review] > Part 3. imgRequestProxy should use an event target derived from the loading > document., v2 > > Review of attachment 8866507 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: image/imgRequestProxy.cpp > @@ +280,5 @@ > > + docGroup = aLoadingDocument->GetDocGroup(); > > + } > > + > > + if (docGroup) { > > + mEventTarget = docGroup->EventTargetFor(mozilla::TaskCategory::Other); > > nsIDocument implements DispatcherTrait, so we could simply this as > if (aLoadingDocument) { > mEventTarget = > aLoadingDocument->EventTargetFor(mozilla::TaskCategory::Other); > } else if (...) { > ... > } I will switch to that, thanks.

Removed some unnecessary code changes, left over from previous attempts.

Incorporate feedback.

Add missing necessary script blockers because even if we are simply cancelling a request, there may be notifications triggered that mutate the list. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d902fed7e3408260bc24cc5b20f354c9ffe4ac

Do you think you can get to these soon, Timothy?

Note I will add updated patches today to use SchedulerGroup::IsSafeToRun() instead of relying upon nsIEventTarget::IsOnCurrentThread.

See part 3 (coming up shortly!) for how I intend to use this. In short, how can I know I am in an unlabeled context given there is no scheduler group associated with that? I confirmed I hit the case of having a listener without a document group on startup and when a new window is opened (as a minimum). Doesn't seem to happen during regular browsing from my quick tests. Without this API we will be forced to dispatch whenever we have an unlabeled listener just to make sure we really are in an unlabeled context, lest we touch a document in the wrong context inadvertently.

Updated comments.

See comment 27. Also fixed a null pointer deref if/when we need to dispatch. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80b0b616ecec84d9f9271e7d6c15a2c98a662760

(In reply to Andrew Osmond [:aosmond] from comment #29) > Created attachment 8875768 [details] [diff] [review] > Part 3. imgRequestProxy should use an event target derived from the loading > document., v4 > > See comment 27. Also fixed a null pointer deref if/when we need to dispatch. > > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=80b0b616ecec84d9f9271e7d6c15a2c98a662760 image/test/mochitest/test_removal_ondecode.html is failing because some events come in after we finish the test. This is probably because of the changes in part 6 attachment 8866800 [details] [diff] [review]. I will fix the test. (Also, the description for part 6 is confusing. " ... should not associate ... with the same document." really ought to be "with the same request.").

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b136918611befa8337526ac8dab147146de8959

Comment on attachment 8866798 [details] [diff] [review] Part 1. ProgressTracker should select an event target from observers and expose to callers., v3 Can you document on the declaration of mEventTarget how it is used. You're changing the meaning of the mutex here. You should change the name of the mutex and the comment above it. You should move all the variables protected by the mutex to be declared together so it is clear which variables need to be protected when used.

Comment on attachment 8866798 [details] [diff] [review] Part 1. ProgressTracker should select an event target from observers and expose to callers., v3 Since mEventTarget is never null can we make it a NotNull?

Comment on attachment 8875766 [details] [diff] [review] Part 0. Add SchedulerGroup::IsSafeToRunUnlabelled method to determine if running in an unlabeled context., v1 Review of attachment 8875766 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/SchedulerGroup.h @@ +68,5 @@ > > + // This function returns true if it's currently safe to run unlabeled code > + // with no known SchedulerGroup. It will only return true if we're inside an > + // unlabeled runnable. > + static bool IsSafeToRunUnlabelled() Please use the (weird) US spelling: unlabeled.

Comment on attachment 8875767 [details] [diff] [review] Part 2. IDecodingTask should use the event target from ProgressTracker for main thread runnables., v3 >+ // While we probably have the correct event target for the narrowest scheduler >+ // group, we don't have the scheduler group itself to verify our context. >+ // However the only time this would matter is if we did a synchronous decode >+ // on the main thread -- it is most likely that the scheduler group context >+ // matches the current execution context in that case. Even if it does not, we >+ // will do a dispatch for the particular imgRequestProxy at notification time, >+ // which is what we would have done here anyways had we been able to know. What does this mean? This _is_ notification time because we only call it when we have a notification to send. I don't like the naming of IsOnEventTarget. It is not clear that it has side effects, and basically must be called before sending a notification. Do we even need to have a mEventTarget member variable since we recompute it every time we send a notification?

(In reply to Timothy Nikkel (:tnikkel) from comment #35) > Comment on attachment 8875767 [details] [diff] [review] > Part 2. IDecodingTask should use the event target from ProgressTracker for > main thread runnables., v3 > > >+ // While we probably have the correct event target for the narrowest scheduler > >+ // group, we don't have the scheduler group itself to verify our context. > >+ // However the only time this would matter is if we did a synchronous decode > >+ // on the main thread -- it is most likely that the scheduler group context > >+ // matches the current execution context in that case. Even if it does not, we > >+ // will do a dispatch for the particular imgRequestProxy at notification time, > >+ // which is what we would have done here anyways had we been able to know. > > What does this mean? This _is_ notification time because we only call it > when we have a notification to send. > > I don't like the naming of IsOnEventTarget. It is not clear that it has side > effects, and basically must be called before sending a notification. Do we > even need to have a mEventTarget member variable since we recompute it every > time we send a notification? I will try to clarify the comment and think of a better name... There are two levels of notifications that we need to consider, one in IDecodingTask to its listener (the image), and one above us in ProgressTracker (imgRequestProxy). My belief is that we do need to cache the event target, at least to cover the most pathological scenarios. ProgressTracker::mEventTarget's effective scheduler group changes depending on what imgRequestProxy's are attached. Going between the unlabelled group (referred to as group U) and doc group A is fine -- if the first event E1 is dispatched on U, and the second event E2 on A, we know the execution order is correct due to the rules, as is E1 on A then E2 on U. However if we replace group U with the system group (S) or a different doc group (B), then we cannot predict the order of events, because unlike the unlabelled group, they have no scheduling restrictions relative to each other. The way I chose to solve this is to cache the event target the first time we have an event to dispatch. That way we are consistently in the same event queue and our order of events is guaranteed. Most of the time this will do the right thing, and in the pathological cases where different documents with different imgRequestProxys care about an image, it will at worst delay the events a bit longer (because maybe we defaulted to a group that was backgrounded) and cause a few extra dispatches at the second level of notifications in ProgressTracker.

(In reply to Andrew Osmond [:aosmond] from comment #36) > (In reply to Timothy Nikkel (:tnikkel) from comment #35) > > Comment on attachment 8875767 [details] [diff] [review] > > Part 2. IDecodingTask should use the event target from ProgressTracker for > > main thread runnables., v3 > > > > >+ // While we probably have the correct event target for the narrowest scheduler > > >+ // group, we don't have the scheduler group itself to verify our context. > > >+ // However the only time this would matter is if we did a synchronous decode > > >+ // on the main thread -- it is most likely that the scheduler group context > > >+ // matches the current execution context in that case. Even if it does not, we > > >+ // will do a dispatch for the particular imgRequestProxy at notification time, > > >+ // which is what we would have done here anyways had we been able to know. > > > > What does this mean? This _is_ notification time because we only call it > > when we have a notification to send. > > > > I don't like the naming of IsOnEventTarget. It is not clear that it has side > > effects, and basically must be called before sending a notification. Do we > > even need to have a mEventTarget member variable since we recompute it every > > time we send a notification? > > I will try to clarify the comment and think of a better name... There are > two levels of notifications that we need to consider, one in IDecodingTask > to its listener (the image), and one above us in ProgressTracker > (imgRequestProxy). > > My belief is that we do need to cache the event target, at least to cover > the most pathological scenarios. ProgressTracker::mEventTarget's effective > scheduler group changes depending on what imgRequestProxy's are attached. > Going between the unlabelled group (referred to as group U) and doc group A > is fine -- if the first event E1 is dispatched on U, and the second event E2 > on A, we know the execution order is correct due to the rules, as is E1 on A > then E2 on U. However if we replace group U with the system group (S) or a > different doc group (B), then we cannot predict the order of events, because > unlike the unlabelled group, they have no scheduling restrictions relative > to each other. > > The way I chose to solve this is to cache the event target the first time we > have an event to dispatch. That way we are consistently in the same event > queue and our order of events is guaranteed. Most of the time this will do > the right thing, and in the pathological cases where different documents > with different imgRequestProxys care about an image, it will at worst delay > the events a bit longer (because maybe we defaulted to a group that was > backgrounded) and cause a few extra dispatches at the second level of > notifications in ProgressTracker. Oh, I should say to go from A to B, you need to remove the request of A, because when we have multiple scheduler groups, we of course default to the unlabelled group. As for other approaches -- as I recall, I tried sticking with the unlabelled group if it was ever selected, but it was actually common for an image to move document groups. When you open a new tab (using the menu or ctrl+T), it will display suggested pages as tiles, and those can cause the same image to get a new imgRequestProxy for a new group. I was hoping to keep that case efficient. I'm open to simplifications of course. If we remove the use of the system group is one option, then we only need to worry about document group conflict scenarios. When it comes to threads, I tend to obsess over locks and scheduling, so maybe I'm being overly paranoid :).

Fix spelling. Funny I did it "right" in all the comments, but did not even notice the function name.

Make mEventTarget NotNull<nsCOMPtr<nsIEventTarget>>. Update comments in ProgressTracker::RemoveObserver based on changes in part 3.

Split IDecodingTask::EnsureHasEventTarget from IDecodingTask::IsOnEventTarget. Write (hopefully) better comments on what is going on.

Avoid holding onto the event target when we have the tab group (as added in v4). This revealed that the event targets actually get cleared for a tab group before our request gets released on shutdown (and in theory in other cases too). Now we return the unlabelled group in that case too. Hopefully improved comments.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f815879f609de3be61c35baf0aef47a73999d0

No functional change, just fix broken language in comments.

Fix another test with the same problem as previous. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27079cf17e0f34bd0f3c078cdb0a0824f05971fb

Actually incorporate all review feedback on part 1 this time. No functional change but rename mImageMutex to mMutex, and document things properly.

Comment on attachment 8876166 [details] [diff] [review] Part 3. imgRequestProxy should use an event target derived from the loading document., v6 >+void >+imgRequestProxy::AddProxy(nsIDocument* aLoadingDocument) An AddProxy function on imgRequestProxy doesn't make sense, imgRequestProxy is the proxy. How about AddToOwner? RegisterWithOwner? >@@ -830,17 +951,25 @@ imgRequestProxy::OnLoadComplete(bool aLastPart) > // There's all sorts of stuff here that could kill us (the OnStopRequest call > // on the listener, the removal from the loadgroup, the release of the > // listener, etc). Don't let them do it. > nsCOMPtr<imgIRequest> kungFuDeathGrip(this); > > if (mListener && !mCanceled) { > // Hold a ref to the listener while we call it, just in case. > nsCOMPtr<imgINotificationObserver> listener(mListener); >- listener->Notify(this, imgINotificationObserver::LOAD_COMPLETE, nullptr); >+ if (IsOnEventTarget()) { >+ listener->Notify(this, imgINotificationObserver::LOAD_COMPLETE, nullptr); >+ } else { >+ nsCOMPtr<imgIRequest> self(this); >+ Dispatch(NS_NewRunnableFunction("imgRequestProxy::OnLoadComplete", >+ [listener, self]() -> void { >+ listener->Notify(self, imgINotificationObserver::LOAD_COMPLETE, nullptr); >+ })); >+ } > } Since this doesn't return, aren't we going to execute the rest of OnLoadComplete twice? >@@ -945,17 +1100,17 @@ imgRequestProxy::GetStaticRequest(imgRequestProxy** aReturn) > // We are animated. We need to create a frozen version of this image. > RefPtr<Image> frozenImage = ImageOps::Freeze(image); > > // Create a static imgRequestProxy with our new extracted frame. > nsCOMPtr<nsIPrincipal> currentPrincipal; > GetImagePrincipal(getter_AddRefs(currentPrincipal)); > RefPtr<imgRequestProxy> req = new imgRequestProxyStatic(frozenImage, > currentPrincipal); >- req->Init(nullptr, nullptr, mURI, nullptr); >+ req->Init(nullptr, nullptr, nullptr, mURI, nullptr); Isn't there a document we can use for static requests? Or do you add that in a later patch? > /* Do the proper refcount management to null out mListener */ >- void NullOutListener(); >+ void NullOutListener(bool aKeepTabGroup); Whereever you call NullOutListener can you add /* aKeepTabGroup = */ so it's clear what true/false means at the callsite. Have you looked at how often we end up having to dispatch a runnable because we are on the wrong event target? Particularly OnLoadComplete, which I would guess comes via network code so we aren't likely on the right event target there. I'd like to avoid delaying these events.

Comment on attachment 8866800 [details] [diff] [review] Part 6. nsImageLoadingContent should not associate scripted and XPCOM observers with the same document., v2 >+nsImageLoadingContent::AddObserver(imgINotificationObserver* aObserver) >+{ >+ NS_ENSURE_ARG_POINTER(aObserver); >+ >+ // We want to delay executing the script notifications for both pending and >+ // current until after we have finished inserting the observer into the list. >+ // We can't just clone after insertion because the notifications from the >+ // current request may remove us from the observer list before we clone the >+ // pending request. >+ nsAutoScriptBlocker scriptBlocker; So this is because the Clone call sends out img notifications and then script changes the observer list? These notifications presumably come from the SyncNotifyListener call in imgRequestProxy::PerformClone. Which the comment there says we want to stop doing. So since these are new Clone calls that we are adding here (so they can't be depending on that behaviour), can we make the notifications async for these Clone calls? >+ if (!mScriptedObserverList) { >+ mScriptedObserverList = observer; >+ } else { >+ ScriptedImageObserver* last = mScriptedObserverList; >+ while (last->mNext) { >+ last = last->mNext; >+ } >+ >+ last->mNext = observer; >+ } Is there any reason to put this at the end of the list? >+nsImageLoadingContent::ClearScriptedRequests(int32_t aRequestType, nsresult aReason) >+{ >+ // We want to delay executing the script notifications for the old request >+ // until after we have finished cancelling the clones. Cancellation itself can >+ // trigger notifications which may mutate the observer list before we advance >+ // to the next observer. >+ nsAutoScriptBlocker scriptBlocker; How does it trigger notifications?

(In reply to Timothy Nikkel (:tnikkel) from comment #46) > Comment on attachment 8876166 [details] [diff] [review] > Part 3. imgRequestProxy should use an event target derived from the loading > document., v6 > > >+void > >+imgRequestProxy::AddProxy(nsIDocument* aLoadingDocument) > > An AddProxy function on imgRequestProxy doesn't make sense, imgRequestProxy > is the proxy. How about AddToOwner? RegisterWithOwner? > Fixed (used AddToOwner). > >@@ -830,17 +951,25 @@ imgRequestProxy::OnLoadComplete(bool aLastPart) > > // There's all sorts of stuff here that could kill us (the OnStopRequest call > > // on the listener, the removal from the loadgroup, the release of the > > // listener, etc). Don't let them do it. > > nsCOMPtr<imgIRequest> kungFuDeathGrip(this); > > > > if (mListener && !mCanceled) { > > // Hold a ref to the listener while we call it, just in case. > > nsCOMPtr<imgINotificationObserver> listener(mListener); > >- listener->Notify(this, imgINotificationObserver::LOAD_COMPLETE, nullptr); > >+ if (IsOnEventTarget()) { > >+ listener->Notify(this, imgINotificationObserver::LOAD_COMPLETE, nullptr); > >+ } else { > >+ nsCOMPtr<imgIRequest> self(this); > >+ Dispatch(NS_NewRunnableFunction("imgRequestProxy::OnLoadComplete", > >+ [listener, self]() -> void { > >+ listener->Notify(self, imgINotificationObserver::LOAD_COMPLETE, nullptr); > >+ })); > >+ } > > } > > Since this doesn't return, aren't we going to execute the rest of > OnLoadComplete twice? > It only dispatches the call to imgINotificationObserver::Notify, it does not run imgRequestProxy::OnLoadComplete again. The only difference is that the ::Notify call happens after the accounting done below (obviously, due to the dispatch). > >@@ -945,17 +1100,17 @@ imgRequestProxy::GetStaticRequest(imgRequestProxy** aReturn) > > // We are animated. We need to create a frozen version of this image. > > RefPtr<Image> frozenImage = ImageOps::Freeze(image); > > > > // Create a static imgRequestProxy with our new extracted frame. > > nsCOMPtr<nsIPrincipal> currentPrincipal; > > GetImagePrincipal(getter_AddRefs(currentPrincipal)); > > RefPtr<imgRequestProxy> req = new imgRequestProxyStatic(frozenImage, > > currentPrincipal); > >- req->Init(nullptr, nullptr, mURI, nullptr); > >+ req->Init(nullptr, nullptr, nullptr, mURI, nullptr); > > Isn't there a document we can use for static requests? Or do you add that in > a later patch? > This was an oversight. Updated part 3, and later parts forthcoming. > > /* Do the proper refcount management to null out mListener */ > >- void NullOutListener(); > >+ void NullOutListener(bool aKeepTabGroup); > > Whereever you call NullOutListener can you add /* aKeepTabGroup = */ so it's > clear what true/false means at the callsite. > Done. > > Have you looked at how often we end up having to dispatch a runnable because > we are on the wrong event target? Particularly OnLoadComplete, which I would > guess comes via network code so we aren't likely on the right event target > there. I'd like to avoid delaying these events. My recollection was that we rarely had event target conflicts. Testing again reveals it happens a lot more than I would expect. I will look into this further and give you a proper answer in the next day.

Updated as per feedback in part 3.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=309eaa336a9d325fc70a5dcca2326e6abf5e2846 (In reply to Timothy Nikkel (:tnikkel) from comment #48) > Comment on attachment 8866800 [details] [diff] [review] > Part 6. nsImageLoadingContent should not associate scripted and XPCOM > observers with the same document., v2 > > >+nsImageLoadingContent::AddObserver(imgINotificationObserver* aObserver) > >+{ > >+ NS_ENSURE_ARG_POINTER(aObserver); > >+ > >+ // We want to delay executing the script notifications for both pending and > >+ // current until after we have finished inserting the observer into the list. > >+ // We can't just clone after insertion because the notifications from the > >+ // current request may remove us from the observer list before we clone the > >+ // pending request. > >+ nsAutoScriptBlocker scriptBlocker; > > So this is because the Clone call sends out img notifications and then > script changes the observer list? These notifications presumably come from > the SyncNotifyListener call in imgRequestProxy::PerformClone. Which the > comment there says we want to stop doing. So since these are new Clone calls > that we are adding here (so they can't be depending on that behaviour), can > we make the notifications async for these Clone calls? > Is it worth the trouble to make an exception here given how few users there are of these APIs? > >+ if (!mScriptedObserverList) { > >+ mScriptedObserverList = observer; > >+ } else { > >+ ScriptedImageObserver* last = mScriptedObserverList; > >+ while (last->mNext) { > >+ last = last->mNext; > >+ } > >+ > >+ last->mNext = observer; > >+ } > > Is there any reason to put this at the end of the list? > Hm, no :). Fixed! > >+nsImageLoadingContent::ClearScriptedRequests(int32_t aRequestType, nsresult aReason) > >+{ > >+ // We want to delay executing the script notifications for the old request > >+ // until after we have finished cancelling the clones. Cancellation itself can > >+ // trigger notifications which may mutate the observer list before we advance > >+ // to the next observer. > >+ nsAutoScriptBlocker scriptBlocker; > > How does it trigger notifications? I think I was concerned about the following call path: imgRequestProxy::CancelAndForgetObserver imgRequest::RemoveProxy imgRequest::Cancel imgRequest::ContinueCancel ProgressTracker::SyncNotifyProgress

(In reply to Andrew Osmond [:aosmond] from comment #41) > Created attachment 8876152 [details] [diff] [review] > Part 3. imgRequestProxy should use an event target derived from the loading > document., v5 > > Avoid holding onto the event target when we have the tab group (as added in > v4). This revealed that the event targets actually get cleared for a tab > group before our request gets released on shutdown (and in theory in other > cases too). Now we return the unlabelled group in that case too. Hopefully > improved comments. I was so concerned about the TabGroup that I forgot about the case where we have a listener, but no document or tab group (it should return do_GetMainThread target). This will cause bugs, maybe this is related to why I see so many more target mismatches as alluded to in comment 49. Working on a fix.

Add a new assert to ensure ProgressTracker::mObserversWithTargets does not exceed the number of observers in the mObservers. Only adjust mObserversWithTargets if the entry was actually removed from the table.

Reverted back to caching the event target in imgRequestProxy::mEventTarget. Reason being is that the tab group is unreliable during shutdown on giving us a target, it complicated imgRequestProxy::NullOutListener use, and broke imgRequestProxy::GetEventTarget anyways :). Now the code is simpler and far more predictable, fixing the worst of the target mismatches during ProgressTracker::AddObserver. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9afc299b583ea68fa672b74780ea6d97932b6957 (In reply to Andrew Osmond [:aosmond] from comment #52) > I was so concerned about the TabGroup that I forgot about the case where we > have a listener, but no document or tab group (it should return > do_GetMainThread target). This will cause bugs, maybe this is related to why > I see so many more target mismatches as alluded to in comment 49. Working on > a fix. Now I can properly answer the question on how often imgRequestProxy::Dispatch happens: you can make it happen, but it is rare. 1) It is much easier to do by first reducing dom.ipc.processCount to 1, because otherwise even if the same image is loaded, it is often in a different process anyways. 2) Even then, I only observed it when closing tabs, from the following imgRequestProxy::SyncNotifyListener call: http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/image/imgLoader.cpp#2839 There were usually two proxies in imgCacheValidator::mProxies, and the dispatch got triggered on the second proxy. This is probably because the network request which triggered this had a good first guess as to which document the image belonged to (e.g. the first proxy in the list) but failed for the second. There was maybe a 6-12 dispatches triggered when closing a tab which was on a related page, but not part of the same tab group (e.g. explicitly browsed to the same URL after opening a new tab). Based on these observations, I'm not terribly worried about performance implications of the dispatch. But if you want to collect telemetry data on this, I can add another patch to the queue :).

Grr, why did some reordering of object variables in the constructor cause a build failure, and others a warning? try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b54ea3830c34c4cefccf68c267637e3c86606b04

(In reply to Andrew Osmond [:aosmond] from comment #36) > The way I chose to solve this is to cache the event target the first time we > have an event to dispatch. That way we are consistently in the same event > queue and our order of events is guaranteed. Most of the time this will do > the right thing, and in the pathological cases where different documents > with different imgRequestProxys care about an image, it will at worst delay > the events a bit longer (because maybe we defaulted to a group that was > backgrounded) and cause a few extra dispatches at the second level of > notifications in ProgressTracker. Ugh, that's pretty gross. Dispatching the image notifications in a group for a background or dead tab when the active tab needs them. :(

(In reply to Andrew Osmond [:aosmond] from comment #51) > > So this is because the Clone call sends out img notifications and then > > script changes the observer list? These notifications presumably come from > > the SyncNotifyListener call in imgRequestProxy::PerformClone. Which the > > comment there says we want to stop doing. So since these are new Clone calls > > that we are adding here (so they can't be depending on that behaviour), can > > we make the notifications async for these Clone calls? > > > > Is it worth the trouble to make an exception here given how few users there > are of these APIs? Given that it seems to be hard to get rid of that sync notify it would be nice if we didn't make it even harder to do so in the future by adding more code that depended on it. > > >+nsImageLoadingContent::ClearScriptedRequests(int32_t aRequestType, nsresult aReason) > > >+{ > > >+ // We want to delay executing the script notifications for the old request > > >+ // until after we have finished cancelling the clones. Cancellation itself can > > >+ // trigger notifications which may mutate the observer list before we advance > > >+ // to the next observer. > > >+ nsAutoScriptBlocker scriptBlocker; > > > > How does it trigger notifications? > > I think I was concerned about the following call path: > > imgRequestProxy::CancelAndForgetObserver > imgRequest::RemoveProxy > imgRequest::Cancel > imgRequest::ContinueCancel > ProgressTracker::SyncNotifyProgress Okay. The pattern of using nsAutoScriptBlocker isn't great because it doesn't really prevent the observer list from changing. It only prevents js observers from running, C++ observers can still mess us up. So I would like to depend on this as little as possible.

(In reply to Andrew Osmond [:aosmond] from comment #54) > 2) Even then, I only observed it when closing tabs, from the following > imgRequestProxy::SyncNotifyListener call: > > http://searchfox.org/mozilla-central/rev/ > d67ef71097da4d1aa344c9d9c672e49a7228e765/image/imgLoader.cpp#2839 That call site should be fine. If we get there then we've already handed back a proxy to the existing image (and content/layout is using that proxy as if it was the correct image) and we just got word back from the server that the existing image is still current so can continue to be used.

What worries me about all this is that it sounds very fragile. Can we do something to track this so that we don't mess up the labelling with a change in the future? As it's very likely we will be touching this code again in the near future. Also, you should probably do a try run with talos to see if you regressed anything to avoid finding out after landing.

Comment on attachment 8877729 [details] [diff] [review] Part 6. nsImageLoadingContent should not associate scripted and XPCOM observers with the same document., v3 This is fine, the remaining issue is whether we sync notify for these new clone calls.

Comment on attachment 8866512 [details] [diff] [review] Part 8. ScriptedNotificationObserver should only run directly if safe to run scripts., v1 Do we not need to return here after adding the script runner?

(In reply to Timothy Nikkel (:tnikkel) from comment #56) > (In reply to Andrew Osmond [:aosmond] from comment #36) > > The way I chose to solve this is to cache the event target the first time we > > have an event to dispatch. That way we are consistently in the same event > > queue and our order of events is guaranteed. Most of the time this will do > > the right thing, and in the pathological cases where different documents > > with different imgRequestProxys care about an image, it will at worst delay > > the events a bit longer (because maybe we defaulted to a group that was > > backgrounded) and cause a few extra dispatches at the second level of > > notifications in ProgressTracker. > > Ugh, that's pretty gross. Dispatching the image notifications in a group for > a background or dead tab when the active tab needs them. :( Yes :/. I don't think this is any different from what was originally proposed though.

(In reply to Timothy Nikkel (:tnikkel) from comment #59) > What worries me about all this is that it sounds very fragile. Can we do > something to track this so that we don't mess up the labelling with a change > in the future? As it's very likely we will be touching this code again in > the near future. > Telemetry patch forthcoming to track how many imgRequestProxy objects with a listener required a dispatch. > Also, you should probably do a try run with talos to see if you regressed > anything to avoid finding out after landing. Will do.

I realized that if the listener gets removed between the dispatch of the notify runnable, and the actual execution of the runnable, we can call the listener when it isn't expecting to be called. Given there was already confusion in imgRequestProxy::OnLoadComplete and overall brittleness concerns, I think it was poorly structured and inconsistent without a strong need. Instead, it now switches from calling the listeners directly to re-calling the original function -- all other work to do besides update the listener is also deferred. It also logs when we call imgRequestProxy::Dispatch, so that it is clear that we deferred the execution from the audit trail. I think it is worth re-reviewing.

As requested, avoid new synchronous notifications for imgRequestProxy::Clone. Existing uses will be switched to imgRequestProxy::SyncClone, and imgRequestProxy::Clone becomes asynchronous. The XPCOM method imgIRequest::Clone is preserved as synchronous for backwards compat purposes (no users inside gecko).

Do you consider this an improvement? We can get rid of part 8, and merge this into part 6 if so. This is almost never used, so we want to minimize memory overhead in nsImageLoadingContent -- the nsTArray just has a pointer to a header which it allocates if anything gets put in the array, so same overhead as before. Then we just copy the array whenever we need it, which will be rare. Combined with part 3b, we no longer care about callbacks into the nsImageLoadingContent -- it should be safe. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9062d372cfceae664330c3bd55595bcb26b5397

(In reply to Andrew Osmond [:aosmond] from comment #66) > Created attachment 8879970 [details] [diff] [review] > Part 6b. Avoid using nsAutoScriptBlocker, intended as fixup to part 6, v1 > > Do you consider this an improvement? We can get rid of part 8, and merge > this into part 6 if so. This is almost never used, so we want to minimize > memory overhead in nsImageLoadingContent -- the nsTArray just has a pointer > to a header which it allocates if anything gets put in the array, so same > overhead as before. Then we just copy the array whenever we need it, which > will be rare. Combined with part 3b, we no longer care about callbacks into > the nsImageLoadingContent -- it should be safe. > > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=f9062d372cfceae664330c3bd55595bcb26b5397 Also if you think it is worth it, I could make it use CopyOnWrite to optimize it a bit more. It would need some refactoring for this case as we don't want to allocate an array on the heap until we add the first entry (whereas for images, we almost always have somebody listening so that allocate will happen anyways).

Add telemetry probe to track how many of our requests needed to be dispatched because we wanted to notify in the wrong scheduler group context.

Even stronger condition is required now that we use imgRequestProxy::Clone instead of imgRequestProxy::SyncClone for nsImageLoadingContent::AddObserver. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=923e8d5dafdb3ee410e430d8a62cc6b1e6e8def0

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0322c507a57167ba479533527a49cfeebf8115a7 talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=0322c507a57167ba479533527a49cfeebf8115a7

Comment on attachment 8879973 [details] [diff] [review] Part 10. Add telemetry to track how often imgRequestProxy needs to dispatch., v1 >diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json >+ "IMAGE_REQUEST_DISPATCHED": { >+ "record_in_processes": ["main", "content"], >+ "alert_emails": ["gfx-telemetry-alerts@mozilla.com"], >+ "expires_in_version": "never", >+ "kind": "boolean", >+ "description": "Image requests that required dispatching", >+ "bug_numbers": [1359833] >+ }, Some nits that are somewhere between substantial and minor nits: * An interested observer reading the data description (like myself) isn't going to understand what dispatching is. Is it possible for the histogram description to be a little bit more verbose about what this is, perhaps by linking to a source file or documentation URL that explains a little bit more? * In general the description should include *what* and *when*: so "A value is recorded each time an image begins decoding: true if that decoder requires dispatching." * expires-never is historically common; it's ok if the metric will be providing permanent value, but in general it should not be the default. Normally we ask people to start out by taking a measurement for six months (5 releases) or less, and then decide to extend permanently if the measure proves to be valuable. So I'd prefer this to start out as expires_in_version: "62". * It is common during code refactoring to accidentally break telemetry metrics. For metrics that are collected permanently, we ask that you have a unit test that covers the data collection. This is less important for temporary metrics where the team is actively looking at them frequently. * alert_emails isn't just about email but also about identifying the real person who is responsible for the measurement. We ask that alert_emails includes a specific person (in addition to the email alert list, which is fine). That can be you or another team lead. * If this is intended to be permament, please talk to me about whether having the PI team monitor this for you makes sense. We are standing up a service called "mission control" where we automatically monitor release health metrics, and you set thresholds and we alert/file bugs if metrics go outside of your set bounds.

Rebase.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba510a43eafd0ef748c97b1174f961509e82fa83

(In reply to Benjamin Smedberg [:bsmedberg] from comment #71) > Comment on attachment 8879973 [details] [diff] [review] > Part 10. Add telemetry to track how often imgRequestProxy needs to > dispatch., v1 > > >diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json > > >+ "IMAGE_REQUEST_DISPATCHED": { > >+ "record_in_processes": ["main", "content"], > >+ "alert_emails": ["gfx-telemetry-alerts@mozilla.com"], > >+ "expires_in_version": "never", > >+ "kind": "boolean", > >+ "description": "Image requests that required dispatching", > >+ "bug_numbers": [1359833] > >+ }, > > Some nits that are somewhere between substantial and minor nits: > > * An interested observer reading the data description (like myself) isn't > going to understand what dispatching is. Is it possible for the histogram > description to be a little bit more verbose about what this is, perhaps by > linking to a source file or documentation URL that explains a little bit > more? I expanded the comment in imgRequestProxy::~imgRequestProxy and added a reference to this in the telemetry descriptor. Hopefully that explains it sufficiently :). In essence, this telemetry is defensive. imgRequestProxy is used as a helper class for higher levels (DOM, layout) to receive progress notifications for the decoding of an image. These notifications should typically be issued in the correct scheduler group context for the document to which the image belongs. But multiple documents listening on the same image (or listeners without a document) is permitted, and in that case, we will need to dispatch from our own main thread context to the correct scheduler group context to actually do the work of notifying the listener. This should be rare, and we want to monitor this data closely to make sure that is the case. > * In general the description should include *what* and *when*: so "A value > is recorded each time an image begins decoding: true if that decoder > requires dispatching." Done. > * expires-never is historically common; it's ok if the metric will be > providing permanent value, but in general it should not be the default. > Normally we ask people to start out by taking a measurement for six months > (5 releases) or less, and then decide to extend permanently if the measure > proves to be valuable. So I'd prefer this to start out as > expires_in_version: "62". Sounds good to me. We should certainly know by then if our current approach is working, or if we need to rethink it. > * It is common during code refactoring to accidentally break telemetry > metrics. For metrics that are collected permanently, we ask that you have a > unit test that covers the data collection. This is less important for > temporary metrics where the team is actively looking at them frequently. > * alert_emails isn't just about email but also about identifying the real > person who is responsible for the measurement. We ask that alert_emails > includes a specific person (in addition to the email alert list, which is > fine). That can be you or another team lead. Done. We will be watching this closely over the next few releases, as we don't want any nasty surprises :). > * If this is intended to be permament, please talk to me about whether > having the PI team monitor this for you makes sense. We are standing up a > service called "mission control" where we automatically monitor release > health metrics, and you set thresholds and we alert/file bugs if metrics go > outside of your set bounds. I think expiring in 62 is fine for now. It is something that could be useful in the future, but I'm happy to revisit it then if we are concerned.

Comment on attachment 8879965 [details] [diff] [review] Part 3b. Split imgRequestProxy::Clone into Clone and SyncClone., v1 >- // This is wrong!!! We need to notify asynchronously, but there's code that >- // assumes that we don't. This will be fixed in bug 580466. >- clone->SyncNotifyListener(); >+ if (GetOwner() && GetOwner()->GetValidator()) { >+ clone->SetNotificationsDeferred(true); >+ GetOwner()->GetValidator()->AddProxy(clone); >+ } else if (aSyncNotify) { >+ // This is wrong!!! We need to notify asynchronously, but there's code that >+ // assumes that we don't. This will be fixed in bug 580466. Note that if we >+ // have a validator, we won't issue notifications anyways because they are >+ // deferred, so there is no point in requesting. >+ clone->SyncNotifyListener(); >+ } else { >+ // Note that if we have a validator, we don't want to issue notifications >+ // here because we want to defer until that completes and this would >+ // override that. >+ clone->NotifyListener(); >+ } This seems more complex then necessary. I was expecting a simple "if (sync) {} else {}" branch. Can we call NewProxy something else? Based on the name one would expect it to create a new proxy and nothing else, the the static proxy override uses some information from the existing proxy. NewClonedProxy? Or CloneNewProxy?

(In reply to Timothy Nikkel (:tnikkel) from comment #75) > Comment on attachment 8879965 [details] [diff] [review] > Part 3b. Split imgRequestProxy::Clone into Clone and SyncClone., v1 > > >- // This is wrong!!! We need to notify asynchronously, but there's code that > >- // assumes that we don't. This will be fixed in bug 580466. > >- clone->SyncNotifyListener(); > >+ if (GetOwner() && GetOwner()->GetValidator()) { > >+ clone->SetNotificationsDeferred(true); > >+ GetOwner()->GetValidator()->AddProxy(clone); > >+ } else if (aSyncNotify) { > >+ // This is wrong!!! We need to notify asynchronously, but there's code that > >+ // assumes that we don't. This will be fixed in bug 580466. Note that if we > >+ // have a validator, we won't issue notifications anyways because they are > >+ // deferred, so there is no point in requesting. > >+ clone->SyncNotifyListener(); > >+ } else { > >+ // Note that if we have a validator, we don't want to issue notifications > >+ // here because we want to defer until that completes and this would > >+ // override that. > >+ clone->NotifyListener(); > >+ } > > This seems more complex then necessary. I was expecting a simple "if (sync) > {} else {}" branch. > I did it because it is necessary -- I can expand the comment above clone->NotifyListener though if it is unclear :). Calling imgRequestProxy::NotifyListener will in turn call either ProgressTracker::Notify or ProgressTracker::NotifyCurrentState. Both of these call imgRequestProxy::SetNotificationsDeferred(true), and then dispatch AsyncNotifyRunnable and AsyncNotifyCurrentStateRunnable respectively. These runnables trigger the notifications but also call imgRequestProxy::SetNotificationsDeferred(false) which clears our flag prematurely if we have a validator. http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/image/ProgressTracker.cpp#220 http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/image/ProgressTracker.cpp#140 This will trips an assert later: http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/image/imgLoader.cpp#2830 Also, in the original code, calling imgRequestProxy::SyncNotifyListener is very confusing because it doesn't actually *do* anything if imgRequestProxy::SetNotificationsDeferred(true) is called beforehand: http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/image/ProgressTracker.cpp#301 > Can we call NewProxy something else? Based on the name one would expect it > to create a new proxy and nothing else, the the static proxy override uses > some information from the existing proxy. NewClonedProxy? Or CloneNewProxy? Agreed, I'll rename it to NewClonedProxy.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=418f9ae215b1ec9052927024940a956abe4d37c0

Comment on attachment 8887135 [details] [diff] [review] Part 10. Add telemetry to track how often imgRequestProxy needs to dispatch., v2 data-r=me

Latest try exposed image/test/mochitest/test_removal_ondecode.html was actually already really broken before any of my changes. It did not display the first image (and so no onDecodeComplete event came in for that image, onDecodeComplete was also not fired for invalid images where we cannot get the size / fail the metadata decode, and we needed to wait on onDecodeComplete because otherwise we might trigger a DOM event from gContent.appendChild as it was still in the DOM tree, which in turn tripped an NS_ERROR warning we were dropping events due to nsAutoScriptBlocker...). Fixed these shortcomings.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41df9e1daa845c9350fe4d93f66bb1932633dc76

(In reply to Andrew Osmond [:aosmond] from comment #76) > Calling imgRequestProxy::NotifyListener will in turn call either > ProgressTracker::Notify or ProgressTracker::NotifyCurrentState. Both of > these call imgRequestProxy::SetNotificationsDeferred(true), and then > dispatch AsyncNotifyRunnable and AsyncNotifyCurrentStateRunnable > respectively. These runnables trigger the notifications but also call > imgRequestProxy::SetNotificationsDeferred(false) which clears our flag > prematurely if we have a validator. Oh, that's an existing bug in overloading the use of bool flag for SetNotificationsDeferred. The SetNotificationsDeferred that we do in Clone is because we are waiting on the network to tell us if the existing cached image is valid to use. The use of SetNotificationsDeferred in ProgressTracker is for a different purpose. There is a bug open that hits the assert you mention. We should probably separate the flag into two flags.

(In reply to Timothy Nikkel (:tnikkel) from comment #81) > Oh, that's an existing bug in overloading the use of bool flag for > SetNotificationsDeferred. The SetNotificationsDeferred that we do in Clone > is because we are waiting on the network to tell us if the existing cached > image is valid to use. The use of SetNotificationsDeferred in > ProgressTracker is for a different purpose. There is a bug open that hits > the assert you mention. We should probably separate the flag into two flags. But that is a task for another day.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4ec8080af6 Part 0. Add SchedulerGroup::IsSafeToRunUnlabeled method to determine if running in an unlabeled context. r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/fd25a183897f Part 1. ProgressTracker should select an event target from observers and expose to callers. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/8941aa986abc Part 2. IDecodingTask should use the event target from ProgressTracker for main thread runnables. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9a48e8a4fb Part 3a. imgRequestProxy should use an event target derived from the loading document. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/e347c123dad6 Part 3b. Split imgRequestProxy::Clone into Clone and SyncClone. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/30031cf13eae Part 4. imgLoader should pass down the loading document to the imgRequest. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/9117345ddfab Part 5. Callers pass the loading document to imgRequestProxy::SyncClone and GetStaticRequest. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/deb1ed01ba9d Part 6. nsImageLoadingContent should not associate scripted and XPCOM observers with the same document. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c0cf283703 Part 7. nsImageLoadingContent native observers should use the new API. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/b63d906a88b5 Part 8. ScriptedNotificationObserver should use nsAutoScriptBlocker when issuing notifications. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/b44a7fd7a417 Part 9. Fix image mochitests to not assume a particular order of events. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/09deb3ef1f63 Part 10. Add telemetry to track how often imgRequestProxy needs to dispatch. r=tnikkel data-r=bsmedberg

https://hg.mozilla.org/mozilla-central/rev/ae4ec8080af6 https://hg.mozilla.org/mozilla-central/rev/fd25a183897f https://hg.mozilla.org/mozilla-central/rev/8941aa986abc https://hg.mozilla.org/mozilla-central/rev/ca9a48e8a4fb https://hg.mozilla.org/mozilla-central/rev/e347c123dad6 https://hg.mozilla.org/mozilla-central/rev/30031cf13eae https://hg.mozilla.org/mozilla-central/rev/9117345ddfab https://hg.mozilla.org/mozilla-central/rev/deb1ed01ba9d https://hg.mozilla.org/mozilla-central/rev/e1c0cf283703 https://hg.mozilla.org/mozilla-central/rev/b63d906a88b5 https://hg.mozilla.org/mozilla-central/rev/b44a7fd7a417 https://hg.mozilla.org/mozilla-central/rev/09deb3ef1f63

As a followup, initial IMAGE_REQUEST_DISPATCHED results are promising -- only 0.15% of requests require a dispatch to run in a different scheduler group (or ~1 out of 667 requests).