1139804 - Stop accessing private members of imgRequest all over the place

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

imgRequest is a very trusting class; it has tons of friends that freely access its private members. That's a bad position to be in, though, for a class with members that are mutated on multiple threads simultaneously. It's essential that code outside of imgRequest start accessing imgRequest members through accessors.

Several races exist around unsynchronized accesses to imgRequest::mImage, so the most important aspect of this bug is to ensure those accesses use imgRequest::GetImage(), which correctly takes the lock. But I think it's best to just totally eliminate all of these friend relationships, because at this point imgRequest's encapsulation is totally undermined and it makes the code much harder to reason about.

Comment 1

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

Attachment: (Part 1) - Remove references to nonexistent type imgRequestNotifyRunnable

The first friend class we'll remove doesn't even appear anywhere in the code.

Comment 2

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

Attachment: (Part 2) - Stop accessing private imgRequest members in imgRequestProxy

In part 2 we make imgRequestProxy no longer a friend of imgRequest.

In all of these parts the strategy is basically the same. I remove the friend
declaration, make private methods public when necessary, and introduce new
accessors when it's needed. I've tried to clean up the documentation a little
where it's possible (in particular, to make it Doxygen-compatible, which is more
important for public methods). Any major tweaks should happen in another bug,
though.

Comment 3

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

Attachment: (Part 3) - Stop accessing private imgRequest members in imgCache* classes

In part 3, the imgCache* set of classes stop being friends with imgRequest.

Comment 4

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

Attachment: (Part 4) - Stop accessing private imgRequest members in imgLoader

In part 4, we do the same for imgLoader.

Comment 5

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

Attachment: (Part 5) - Stop accessing private imgRequest members in imgMemoryReporter

In part 5, we do the same for imgMemoryReporter.

Comment 6

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

Attachment: (Part 6) - Make imgRequest no longer friends with ProgressTracker

Finally, in part 6 we make imgRequest no longer friends with ProgressTracker. We
don't need to do anything else, because all of the methods that ProgressTracker
needs were already made public in earlier parts.

Comment 7

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

CC'ing folks who might be interested. Accessing the private member variables of imgRequest was causing data races that may be responsible for the test failure in bug 1019840.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8573139 [details] [diff] [review]
(Part 2) - Stop accessing private imgRequest members in imgRequestProxy

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

::: image/src/imgRequest.h
@@ +150,5 @@
> +  /// Returns a non-owning pointer to this imgRequest's MIME type.
> +  const char* GetMimeType() const { return mContentType.get(); }
> +
> +  /**
> +   * @return the priority of the underlying network request, or

I would keep the same pattern for the comments. What about:

/**
 * Returns the priority of the ...

::: image/src/imgRequestProxy.h
@@ +170,5 @@
>    already_AddRefed<ProgressTracker> GetProgressTracker() const;
>  
>    nsITimedChannel* TimedChannel()
>    {
>      if (!GetOwner())

if (!GetOwner()) {
  return nullptr;
}

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8573147 [details] [diff] [review]
(Part 5) - Stop accessing private imgRequest members in imgMemoryReporter

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

::: image/src/imgRequest.h
@@ +195,5 @@
>    void RemoveFromCache();
>  
>    nsIProperties* Properties() const { return mProperties; }
>  
> +  bool HasConsumers();

const?

Comment 10

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

Thanks for the reviews! I'll make those changes.

(In reply to Andrea Marchesini (:baku) from comment #8)
> I would keep the same pattern for the comments. What about:

This is a good idea. I had thought that '///' is a "brief" Doxygen comment, and the '/**' is a "detailed" Doxygen comment. But apparently you can use multiple lines of '///' to make a "detailed" Doxygen comment, so there's no reason not to do so. I'll update the patch.

Comment 11

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

Attachment: (Part 2) - Stop accessing private imgRequest members in imgRequestProxy

Addressed review comments.

Comment 12

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

Attachment: (Part 4) - Stop accessing private imgRequest members in imgLoader

Rebased and addressed review comments.

Comment 13

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

Attachment: (Part 5) - Stop accessing private imgRequest members in imgMemoryReporter

Rebased and addressed review comments.

Comment 14

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

Attachment: (Part 6) - Make imgRequest no longer friends with ProgressTracker

Rebased.

Comment 15

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

Pushed to inbound:

165b9f6bb984	Seth Fowler — Bug 1139804 (Part 6) - Make imgRequest no longer friends with ProgressTracker. r=baku
e488bdbe1bc8	Seth Fowler — Bug 1139804 (Part 5) - Stop accessing private imgRequest members in imgMemoryReporter. r=baku
06eca034b085	Seth Fowler — Bug 1139804 (Part 4) - Stop accessing private imgRequest members in imgLoader. r=baku
61c1f8d6b44a	Seth Fowler — Bug 1139804 (Part 3) - Stop accessing private imgRequest members in imgCache* classes. r=baku
434182bfb8cc	Seth Fowler — Bug 1139804 (Part 2) - Stop accessing private imgRequest members in imgRequestProxy. r=baku
670fa5a3482d	Seth Fowler — Bug 1139804 (Part 1) - Remove references to nonexistent type imgRequestNotifyRunnable. r=baku

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/670fa5a3482d
https://hg.mozilla.org/mozilla-central/rev/434182bfb8cc
https://hg.mozilla.org/mozilla-central/rev/61c1f8d6b44a
https://hg.mozilla.org/mozilla-central/rev/06eca034b085
https://hg.mozilla.org/mozilla-central/rev/e488bdbe1bc8
https://hg.mozilla.org/mozilla-central/rev/165b9f6bb984