seer should not allow subresources/subhosts to have a higher load count than their parent

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: u408661, Assigned: jaypoulz)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
Right now it's conceivable that, if a page (say www.example.com/index.html) loads multiple resources from the same origin (say www.example2.com/style.css and www.example2.com/script.js), then www.example2.com will say it has been loaded twice, while www.example.com (which caused all the loads of www.example2.com) was only loaded once. We should limit the hits for a suborigin/subresource to the number of times the parent was loaded.
Assignee

Comment 1

5 years ago
Posted patch bug-1016617-fix.patch (obsolete) — Splinter Review
Attachment #8430896 - Flags: review?(hurley)
Reporter

Comment 3

5 years ago
Comment on attachment 8430896 [details] [diff] [review]
bug-1016617-fix.patch

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

Just a couple little issues, nothing functional. Fix up the issues, and upload a new patch, and we should be good to go.

::: netwerk/base/src/Seer.cpp
@@ +1939,5 @@
>    }
>    mozStorageStatementScoper scope(stmt);
>  
> +  // Make sure the hit count can't be more that its parent
> +  int32_t hitCount = ((info.hitCount + 1) > parentCount) ?\

You don't need the \ here, C++ will keep scanning until it finds a ;

@@ +1946,3 @@
>    nsresult rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hit_count"),
> +                                      hitCount);
> +  

nit: you added trailing whitespace on this empty line. Get rid of it, please.
Attachment #8430896 - Flags: review?(hurley)
Assignee

Comment 4

5 years ago
Posted patch bug-1016617-fix.patch (obsolete) — Splinter Review
Now with less whitespace. White is saddened.
Attachment #8430896 - Attachment is obsolete: true
Attachment #8430917 - Flags: review?(hurley)
Reporter

Comment 5

5 years ago
Comment on attachment 8430917 [details] [diff] [review]
bug-1016617-fix.patch

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

Sorry, this round's my fault :)

::: netwerk/base/src/Seer.cpp
@@ +1940,5 @@
>    mozStorageStatementScoper scope(stmt);
>  
> +  // Make sure the hit count can't be more that its parent
> +  int32_t hitCount = ((info.hitCount + 1) > parentCount) ?
> +    parentCount : (info.hitCount + 1);

Sorry, I don't know why I didn't think of this last time. Just use std::min here, so

int32_t hitCount = std::min(info.hitCount + 1, parentCount);

instead of reinventing the wheel. This is the last change I'm gonna request from you, I swear :)
Attachment #8430917 - Flags: review?(hurley)
Assignee

Comment 6

5 years ago
Accidentally gave the code an std.

Will add a new try server link for this build.
Attachment #8430917 - Attachment is obsolete: true
Attachment #8430941 - Flags: review?(hurley)
Assignee

Comment 7

5 years ago
Here's a link to the new try build.
https://tbpl.mozilla.org/?tree=Try&rev=19053be91785
Reporter

Updated

5 years ago
Attachment #8430941 - Flags: review?(hurley) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Pushed with some minor tweaks to the commit message. Thanks for the patch!
https://hg.mozilla.org/integration/mozilla-inbound/rev/974979fa171a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/974979fa171a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.