Stop accessing private members of imgRequest all over the place

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(6 attachments, 4 obsolete attachments)

2.48 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.40 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.41 KB, patch
Details | Diff | Splinter Review
8.83 KB, patch
Details | Diff | Splinter Review
5.34 KB, patch
Details | Diff | Splinter Review
892 bytes, patch
Details | Diff | Splinter Review
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.
The first friend class we'll remove doesn't even appear anywhere in the code.
Attachment #8573135 - Flags: review?(amarchesini)
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.
Attachment #8573139 - Flags: review?(amarchesini)
In part 3, the imgCache* set of classes stop being friends with imgRequest.
Attachment #8573141 - Flags: review?(amarchesini)
In part 4, we do the same for imgLoader.
Attachment #8573143 - Flags: review?(amarchesini)
In part 5, we do the same for imgMemoryReporter.
Attachment #8573147 - Flags: review?(amarchesini)
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.
Attachment #8573155 - Flags: review?(amarchesini)
Blocks: 1139818
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.
Attachment #8573135 - Flags: review?(amarchesini) → review+
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;
}
Attachment #8573139 - Flags: review?(amarchesini) → review+
Attachment #8573141 - Flags: review?(amarchesini) → review+
Attachment #8573143 - Flags: review?(amarchesini) → review+
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?
Attachment #8573147 - Flags: review?(amarchesini) → review+
Attachment #8573155 - Flags: review?(amarchesini) → review+
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.
Attachment #8573139 - Attachment is obsolete: true
Rebased and addressed review comments.
Attachment #8573143 - Attachment is obsolete: true
Attachment #8573147 - Attachment is obsolete: true
Attachment #8573155 - Attachment is obsolete: true
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
Depends on: 1150127
No longer depends on: 1150127
Depends on: 1180126
No longer depends on: 1180126
You need to log in before you can comment on or make changes to this bug.