Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NTP Sponsored Images schemaVersion support #4433

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Conversation

petemill
Copy link
Member

Only accepts remote component with known schemaVersion of 1

Fix brave/brave-browser#7923

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill petemill self-assigned this Jan 27, 2020
? std::to_string(*incomingSchemaVersion)
: "missing") <<
", but we expected " << kExpectedSchemaVersion;
images_data_.reset(nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: images_data_.reset() also works.

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have a unit test failure (test crashed):
https://ci.brave.com/job/brave-browser-build-pr/job/ntp-sbw-schema-version/1/execution/node/498/log/

01:31:10 [71/842] NTPSponsoredImagesServiceTest.BasicTest (0 ms)
01:31:10 [ RUN ] NTPSponsoredImagesServiceTest.InternalDataTest
01:31:10 [22069:22069:0127/083109.784849:2870384399:ERROR:ntp_sponsored_images_service.cc(135)] Incoming NTP Sponsored images component data was not valid.Schema version was missing, but we expected 1
01:31:10 ../../brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_service_unittest.cc:45: Failure
01:31:10 Expected: (data) != (nullptr), actual: NULL vs (nullptr)
01:31:10 Stack trace:
01:31:10 #0 0x55ec45888c79 (/home/ubuntu/jenkins/workspace/_build-pr_ntp-sbw-schema-version/src/out/Release/brave_unit_tests+0x4ab9c78)
01:31:10
01:31:10 Received signal 11 SEGV_MAPERR 000000000068
01:31:10 #0 0x55ec45888c79 (/home/ubuntu/jenkins/workspace/_build-pr_ntp-sbw-schema-version/src/out/Release/brave_unit_tests+0x4ab9c78)
01:31:10 r8: 0000000000000000 r9: 00007f68d74d4c80 r10: 7365745f74696e75 r11: 0000000000000000
01:31:10 r12: 00003b355deaf918 r13: 00003b355dfb9730 r14: 00007fffa7d24780 r15: 00003b355deaf980
01:31:10 di: 0000000000000000 si: 00003b355dd5acc0 bp: 00007fffa7d246f0 bx: 00003b355e0cc980
01:31:10 dx: 0000000000000002 ax: 0000000000032520 cx: 00003b355dd5acc8 sp: 00007fffa7d246f0
01:31:10 ip: 000055ec458ba3d4 efl: 0000000000010202 cgf: 0000000000000033 erf: 0000000000000004
01:31:10 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000068
01:31:10 [end of stack trace]
01:31:10 Calling _exit(1). Core file will not be generated.
01:31:10 [72/842] NTPSponsoredImagesServiceTest.InternalDataTest (CRASHED)

@simonhong
Copy link
Member

Fixed above test failure :)

@simonhong simonhong requested a review from bsclifton January 29, 2020 04:56
@bsclifton
Copy link
Member

CI ran into network-audit and test-install intermittent issues, but otherwise passed 😄 (built binaries / unit/browser tests all passed)

@bsclifton bsclifton merged commit 223c062 into master Jan 29, 2020
@bsclifton bsclifton deleted the ntp-sbw-schema-version branch January 29, 2020 08:06
@bsclifton bsclifton added this to the 1.5.x - Nightly milestone Jan 29, 2020
petemill pushed a commit that referenced this pull request Jan 29, 2020
NTP Sponsored Images schemaVersion support
This was referenced Jan 29, 2020
petemill pushed a commit that referenced this pull request Jan 29, 2020
NTP Sponsored Images schemaVersion support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NTP Sponsored Images data should be versioned
3 participants