Fix rewards extension state (notifications) array attempt to set by named property #2407
Conversation
| if (!notifications) { | ||
| notifications = [] | ||
| if (!notifications || Array.isArray(notifications)) { | ||
| notifications = {} |
There was a problem hiding this comment.
This is the main fix. @NejcZdovc can you explain better about:
- why this was an array
- what the repeatable case could be to get in this state where either notification is blank or existing state.notifications is an array (which was my case -
state.notificationswas already[]) so that we can get a test plan
There was a problem hiding this comment.
@petemill line 155 in this same file seems to set it, line 168 merges it back into state.
There was a problem hiding this comment.
I think that I copied it from line 156 where we set it to array and I guess that was problematic as well. So I think we need to fix line 156 as well. You can see that in tests that I added it was expected to be an object 9cecb15#diff-f250acb8ae40c4d514754045d9b6e936R379.
Not sure as why only few people see this problem as it should break for everyone
There was a problem hiding this comment.
@bbondy @NejcZdovc that in L155 also needs to change, right? It's invalid to create this property as an array when it's used as an object.
There was a problem hiding this comment.
@NejcZdovc would only be a problem if state.notifications does not yet exist, right? Do we persist this f/e redux state to disk? Me, personally, I 'upgraded' the profile from dev to nightly before experiencing this bug, if that helps
There was a problem hiding this comment.
The type in L153 also appears to be incorrect, and assume that notification ID is always a number:
let notifications: Record<number, RewardsExtension.Notification> = state.notifications
...but even then it assigns an empty array to that type. I doubt that should pass typescript compile... 🤷♂
There was a problem hiding this comment.
yup we need to change line 155. It's strange why it would work for some and not work for others. Also we had this code (line 155) in there for ages
There was a problem hiding this comment.
I can't remember the specific details, but wondering if this is fallout from the notifications type change. I seem to recall they were initially integers and then switched to strings. I wonder if that caused a problem and/or wasn't fully refactored...
There was a problem hiding this comment.
Confirmed fix works great using my broken profile 😄! I would approve, but I think @NejcZdovc should respond (so that this doesn't get merged on accident)
@NejcZdovc would be good to find when this started, so we know how far back to uplift. I suspect it started with 0.66.x (which is now DEV)
|
@bsclifton this was introduced with brave/brave-browser#2914, so we would need to uplift to 0.64 |
| type: notification.type, | ||
| timestamp: notification.timestamp, | ||
| args: notification.args | ||
| for (const notificationItemData of list) { |
There was a problem hiding this comment.
not sure if this change is needed
There was a problem hiding this comment.
Correct, this was just cleanup to aid readability when diagnosing the bug
There was a problem hiding this comment.
could we revert back to what it was so that change is really minimal as we will probably need to uplift it into beta
There was a problem hiding this comment.
++ on revert back, just to minimize patch (even though it's arguably nicer; I always avoid forEach since you can't break out)
There was a problem hiding this comment.
Actually I was wondering if this was part of the bug in case payload.list is actually the same item in memory as state.notifications, and then it was modifying the list that was being iterating over.... I don't think so though if it's always coming from c++, but I can't vouch for that. I'll revert this, but I did check that this is unchanged down to at least 0.65, and it is unchanged...
petemill
force-pushed
the
fix-rewards-extension-notifications-hang
branch
3 times, most recently
from
329e9dc to
5da66ce
Compare
|
After this fix, my local profile still shows 10 notifications in the badge (and there are 10 notification items in the state), but there are no actual notifications to be seen, in the panel or the rewards settings page. I wonder if that's due to using a production profile in the development build @NejcZdovc ? |
|
let's wait with merge please. Can you please paste array that you received as payload into this reducer. Would like to see what are these 10 notifications? Update: @petemill provided array to me so will check out why this could be a problem and why we wouldn't display them if we get them. If I don't find anything for it in the next few hours I would suggest to merge it and uplift it asap so we remove CPU problem for the users. |
…amed property Causes 100% cpu loop
bsclifton
force-pushed
the
fix-rewards-extension-notifications-hang
branch
from
5da66ce to
eb6313c
Compare
Causes 100% cpu loop
Fix brave/brave-browser#4344
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) onnpm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.