1135977 - Clean up questionable refcounting in imgLoader

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

It's easier to debug things when the code you're debugging is sane and follows modern Mozilla style. Unfortunately, some refcounting code in imgLoader uses some questionable patterns. Let's clean it up.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Use modern Mozilla style for refcounting in imgLoader

In this patch I eliminate NS_ADDREF and NS_RELEASE from imgLoader, and make some
minor simplifications.

Andrea, you and I had talked about you getting involved more with ImageLib, but
unfortunately I haven't had much time until recently to do anything except write
code. Now that things are slowing down for me, are you still interested?

If so, maybe a good starting point is to send you some reviews. =)

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Use threadsafe refcounting for ProxyListener

Here's part 2, which just switches ProxyListener to use threadsafe refcounting.
ProxyListener gets touched by multiple threads, and I'm honestly not sure
whether it's possible for those threads to touch its refcount at the same time,
so it seems better to be safe than sorry. This also makes ProxyListener more in
line with imgRequest, which already uses threadsafe refcounting.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8568301 [details] [diff] [review]
(Part 1) - Use modern Mozilla style for refcounting in imgLoader

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

::: image/src/imgLoader.cpp
@@ +941,5 @@
>    aRequest->GetURI(getter_AddRefs(uri));
>  
>    // init adds itself to imgRequest's list of observers
>    nsresult rv = proxyRequest->Init(aRequest, aLoadGroup, uri, aObserver);
>    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +1543,5 @@
>      mozilla::net::PredictorLearn(aURI, aInitialDocumentURI,
>          nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, aLoadGroup);
>  
>      rv = newChannel->AsyncOpen(listener, nullptr);
> +    if (NS_SUCCEEDED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
  return false;
}

req.forget(aProxyRequest);
return true;

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8568302 [details] [diff] [review]
(Part 2) - Use threadsafe refcounting for ProxyListener

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

I don't like this for these reasons:

1. first of all if we have cross-thread references we should see crashes in debug builds.
We don't have such kind of crashes, as far as I know, so we don't need a thread-safe refcnt.

2. if we move this object between threads, are we sure that we touch internal variables correctly?

Can you answer these 2 questions and ask me a new review? Thanks!

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Andrea Marchesini (:baku) from comment #4)
> 1. first of all if we have cross-thread references we should see crashes in
> debug builds.
> We don't have such kind of crashes, as far as I know, so we don't need a
> thread-safe refcnt.

There actually is such a crash. We are not certain how it happens; the reason I'm making the changes in this bug is to make it easier to reason about what the cause is.

> 2. if we move this object between threads, are we sure that we touch
> internal variables correctly?

Yeah, it should be safe. (Unless things are more buggy than we're currently aware of.) ProxyListener only has one member variable, mDestListener, which is only mutated in ProxyListener::OnStartRequest. Necko always delivers OnStartRequest on the main thread, and it starts dispatching OnDataAvailable events only after OnStartRequest completes. OnDataAvailable *can* be delivered off-main-thread, but the main thread must synchronize with whichever thread dispatches OnDataAvailable to signal that OnStartRequest is complete, so there should be no problem.

The only place I'm aware of where there *could* be a problem is that code may be passing around ProxyListener objects on the main thread at the same time that ProxyListener is used off-main-thread to deliver OnDataAvailable. This might be causing concurrent modification of its refcount on multiple threads. By making its refcount atomic, we eliminate this as a possible explanation of a crash that's happening.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Use modern Mozilla style for refcounting in imgLoader

Thanks for the review! I've made the changes you suggested in this new version of part 1.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8568302 [details] [diff] [review]
(Part 2) - Use threadsafe refcounting for ProxyListener

Rerequesting review; let me know if there's anything more I can clarify!

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8568302 [details] [diff] [review]
(Part 2) - Use threadsafe refcounting for ProxyListener

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

Thanks for the explanation!

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Here's a try job:

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

Comment 10

[Seth Fowler [:seth] [:s2h]]

Try looks good! Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c00551ce36e7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7959e9084f6a

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=25f00181e134 since one of this changes caused a perma failure like:

https://treeherder.mozilla.org/logviewer.html#?job_id=7169706&repo=mozilla-inbound

Comment 12

[Seth Fowler [:seth] [:s2h]]

Pretty darn sure that's unrelated to these patches, but since B2G wasn't included in the original try job, I've pushed a new one:

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

Comment 13

[Seth Fowler [:seth] [:s2h]]

As expected, these patches were unrelated. Repushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb170af16e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b92050b94968

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6bb170af16e4
https://hg.mozilla.org/mozilla-central/rev/b92050b94968