Sparkle framework integration #91

simonhong · 2018-04-12T04:37:14Z

Add initial sparkle framework integration.
Update stuff is added to BraveAppController(AppController subclass).

Close brave/brave-browser#159

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

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

RyanJarv · 2018-04-14T21:31:29Z

@simonhong is the process for running the update working for you now or is that still a WIP? Seems to fetch the appcast file for me but doesn't download the update.

We can use docker for testing against omaha-server (just docker-compose up in https://github.com/Crystalnix/omaha-server). I'm working to get some testing and automation around the update process so that may help as well when that's ready.

RyanJarv · 2018-04-14T21:38:23Z

browser/brave_app_controller_mac.mm

+
+- (void)initializeBraveUpdater {
+  constexpr int kBraveUpdateCheckInterval = 60;
+  NSURL* url = [NSURL URLWithString:@"https://example.com"];


~~Can we get this changed to http://localhost:9090 for testing?~~
Or actually think it makes sense to just remove it for now (we can set these settings in the Info.plist file).

Done. you can change feed url in the `src/brave/brave_init_settings.gni'

simonhong

@darkdh , PTAL.
@bbondy , I need to support for creating subrepo for vendor/sparkle. Please check above description.

simonhong · 2018-04-16T04:00:40Z

browser/brave_app_controller_mac.mm

+
+- (void)initializeBraveUpdater {
+  constexpr int kBraveUpdateCheckInterval = 60;
+  NSURL* url = [NSURL URLWithString:@"https://example.com"];


Done. you can change feed url in the `src/brave/brave_init_settings.gni'

simonhong · 2018-04-16T04:14:17Z

@RyanJarv I implemented SUUpdateDelegate protocol to see error log.
When we have subrepo for 'vendor/sparkle`, this pr would be merged after review.
I think there isn't much more to enable update in client side. So let's check error log.
I'll also install docker for test.

--enable-brave-update-test command argument is introduced.
We can enable brave update by running
out/Release/Brave.app/Contents/MacOS/Brave --enable-brave-update-test.

Also Brave Update menu item is introduced in main menu to test easily.

When I request update with latest PR by update menu, it complains three things.
DSA signing, https, and invalid update info.
I did DSA signing with locally generated dsa_pub.pem and first complain is gone.
For second complain, I changed to use https://localhost:9090.
For last complain, it would be caused because I don’t setup local appcast server.

RyanJarv · 2018-04-16T22:36:59Z

Ok cool this seems to work for me. On second thought go ahead and ping me if your going to try testing with docker since there would be a few changes that need to be made, going to see if I can make this a bit easier.

RyanJarv · 2018-04-17T01:41:37Z

@simonhong don't think you need to make a subrepo for sparkle, dependencies for brave/vendor seem to be managed here: https://github.com/brave/brave-core/blob/master/DEPS#L3

bbondy · 2018-04-17T01:59:37Z

I created a repo for brave/sparkle that you can use, but yes you'd list the dependency to that repo there.

simonhong · 2018-04-17T02:20:03Z

@bbondy Thanks for creating sparkle repo!
I wanted additional repo for sparkle for adding into brave/DEPS like you mentioned :)
@RyanJarv If we need to add dsa_pub.pem to our bundle, please send pub key to me.
I'll update this PR to bundle it.

RyanJarv · 2018-04-17T02:32:40Z

Ah ok, misunderstood the repo thing. Just fyi if you want to test against the server you can use this https://github.com/RyanJarv/updater (let me know if you run into any issues).

Sounds good, will do.

simonhong · 2018-04-17T12:01:20Z

DEPS

@@ -9,6 +9,7 @@ deps = {
  "vendor/requests": "https://github.com/kennethreitz/requests@e4d59bedfd3c7f4f254f4f5d036587bcd8152458",
  "vendor/boto": "https://github.com/boto/boto@f7574aa6cc2c819430c1f05e9a1a1a666ef8169b",
  "vendor/python-patch": "https://github.com/svn2github/python-patch@a336a458016ced89aba90dfc3f4c8222ae3b1403",
+  "vendor/sparkle": "https://github.com/simonhong/Sparkle.git@e6c544e72a72e96d1712f0f503d61ea267bc6e22",


Works well with my personal repo.
I'll update this with brave/sparkle when it is ready.

Will want to have GN set up to build this, shouldn't be pulling in binaries unless we have that being built in some other CI process that we can audit.

(apologies again for the confusion)

darkdh · 2018-04-17T22:44:22Z

patches/build-mac-tweak_info_plist.py.patch

+  # BRAVE(shong): Add sparkle update support
+  if options.brave_feed_url:
+    plist['SUFeedURL'] = options.brave_feed_url
+    plist['SUPublicDSAKeyFile'] = 'dsa_pub.pem'


can we put this into gni?

defined dsa file name in brave_init_settings.gni.

simonhong · 2018-04-18T06:21:44Z

patches/build-mac-tweak_info_plist.py.patch

+  # BRAVE(shong): Add sparkle update support
+  if options.brave_feed_url:
+    plist['SUFeedURL'] = options.brave_feed_url
+    plist['SUPublicDSAKeyFile'] = 'dsa_pub.pem'


defined dsa file name in brave_init_settings.gni.

simonhong · 2018-04-18T06:23:17Z

DEPS

@@ -9,6 +9,7 @@ deps = {
  "vendor/requests": "https://github.com/kennethreitz/requests@e4d59bedfd3c7f4f254f4f5d036587bcd8152458",
  "vendor/boto": "https://github.com/boto/boto@f7574aa6cc2c819430c1f05e9a1a1a666ef8169b",
  "vendor/python-patch": "https://github.com/svn2github/python-patch@a336a458016ced89aba90dfc3f4c8222ae3b1403",
+  "vendor/sparkle": "https://github.com/simonhong/Sparkle.git@3364c87a25a93cf86dc9dd68066830a418ba0d90",


@bbondy @RyanJarv Still I can't use brave/sparkle repo.
So, I tested with my personal repo to build sparkle from its source.
Just three files(BUILD.gn, build_sparkle_framework.py and dsa_pub.pem) are added with unmodified sparkle source.
Also 1.18.1 version is used.
When latest release verson (1.19.0) is used, sometimes crashed with below log.

2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! 2018-04-18 16:49:29.448 Brave[70310:4028787] pid(70310)/euid(501) is calling TIS/TSM in non-main thread environment, ERROR : This is NOT allowed. Please call TIS/TSM in main thread!!! [70310:43015:0418/164929.449313:FATAL:sequenced_task_runner_handle.cc(52)] Check failed: ThreadTaskRunnerHandle::IsSet(). Error: This caller requires a sequenced context (i.e. the current task needs to run from a SequencedTaskRunner). 0 libbase.dylib 0x00000001200caf43 base::debug::StackTrace::StackTrace() + 19 1 libbase.dylib 0x00000001200f1ae0 logging::LogMessage::~LogMessage() + 224 2 libbase.dylib 0x00000001201711b3 base::SequencedTaskRunnerHandle::Get() + 307 3 libbase.dylib 0x000000012017a506 base::Timer::PostNewScheduledTask(base::TimeDelta) + 102 4 libbase.dylib 0x00000001200dfeb0 base::ImportantFileWriter::ScheduleWrite(base::ImportantFileWriter::DataSerializer*) + 208 5 libprefs.dylib 0x0000000134922930 JsonPrefStore::ScheduleWrite(unsigned int) + 96 6 libprefs.dylib 0x000000013492468d JsonPrefStore::ReportValueChanged(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned int) + 317 7 libchrome_dll.dylib 0x000000010d85c61e SegregatedPrefStore::ReportValueChanged(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned int) + 62 8 libprefs.dylib 0x0000000134959869 PrefService::ReportUserPrefChanged(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 105 9 libprefs.dylib 0x000000013496e6f2 subtle::ScopedUserPrefUpdateBase::Notify() + 50 10 libprefs.dylib 0x000000013496e6a9 subtle::ScopedUserPrefUpdateBase::~ScopedUserPrefUpdateBase() + 25 11 libchrome_dll.dylib 0x0000000113800879 ScopedUserPrefUpdate<base::DictionaryValue, (base::Value::Type)6>::~ScopedUserPrefUpdate() + 25 12 libchrome_dll.dylib 0x00000001137ffe75 ScopedUserPrefUpdate<base::DictionaryValue, (base::Value::Type)6>::~ScopedUserPrefUpdate() + 21 13 libchrome_dll.dylib 0x0000000113800849 ScopedUserPrefUpdate<base::DictionaryValue, (base::Value::Type)6>::~ScopedUserPrefUpdate() + 25 14 libchrome_dll.dylib 0x00000001138ac9ab -[BrowserWindowController(Private) saveWindowPositionIfNeeded] + 2939 15 libchrome_dll.dylib 0x000000011389b3d6 -[BrowserWindowController windowDidBecomeMain:] + 86 16 CoreFoundation 0x00007fff4776df2c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12 17 CoreFoundation 0x00007fff4776ddfa _CFXRegistrationPost + 458 18 CoreFoundation 0x00007fff4776db31 ___CFXNotificationPost_block_invoke + 225 19 CoreFoundation 0x00007fff4772b8d0 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1664 20 CoreFoundation 0x00007fff4772aa07 _CFXNotificationPost + 599 21 Foundation 0x00007fff4982d8c7 -[NSNotificationCenter postNotificationName:object:userInfo:] + 66 22 AppKit 0x00007fff44e15c0b -[NSWindow _changeKeyAndMainLimitedOK:] + 940 23 AppKit 0x00007fff44e90ca8 -[NSWindow _orderOutAndCalcKeyWithCounter:stillVisible:docWindow:] + 1261 24 AppKit 0x00007fff44e18c0c NSPerformVisuallyAtomicChange + 146 25 AppKit 0x00007fff455c6833 -[NSWindow _doWindowOrderOutWithWithKeyCalc:forCounter:orderingDone:docWindow:] + 85 26 AppKit 0x00007fff455c7d26 -[NSWindow _reallyDoOrderWindowOutRelativeTo:findKey:forCounter:force:isModal:] + 446 27 AppKit 0x00007fff44e208ec -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 172 28 AppKit 0x00007fff44e1f765 -[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 287 29 AppKit 0x00007fff44e1f5cb -[NSWindow orderWindow:relativeTo:] + 169 30 AppKit 0x00007fff455c540e -[NSWindow _finishClosingWindow] + 479 31 AppKit 0x00007fff44f64ed8 -[NSWindow _close] + 378 32 Sparkle 0x000000010a856b5f -[SUUserInitiatedUpdateDriver closeCheckingWindow] + 122 33 Sparkle 0x000000010a857068 -[SUUserInitiatedUpdateDriver abortUpdateWithError:] + 47 34 Sparkle 0x000000010a82b7b2 -[SUAppcast reportError:] + 688 35 Sparkle 0x000000010a82a34e -[SUAppcast downloaderDidFailWithError:] + 63 36 Sparkle 0x000000010a8377f5 -[SPUDownloaderSession URLSession:task:didCompleteWithError:] + 96 37 CFNetwork 0x00007fff46841c8f __51-[NSURLSession delegate_task:didCompleteWithError:]_block_invoke.182 + 80 38 Foundation 0x00007fff498635df __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 7 39 Foundation 0x00007fff49863441 -[NSBlockOperation main] + 68 40 Foundation 0x00007fff498618ee -[__NSOperationInternal _start:] + 778 41 libdispatch.dylib 0x00007fff6fa38e08 _dispatch_client_callout + 8 42 libdispatch.dylib 0x00007fff6fa4b38e _dispatch_block_invoke_direct + 317 43 libdispatch.dylib 0x00007fff6fa38e08 _dispatch_client_callout + 8 44 libdispatch.dylib 0x00007fff6fa4b38e _dispatch_block_invoke_direct + 317 45 libdispatch.dylib 0x00007fff6fa4b231 dispatch_block_perform + 109 46 Foundation 0x00007fff4985d8fc __NSOQSchedule_f + 342 47 libdispatch.dylib 0x00007fff6fa38e08 _dispatch_client_callout + 8 48 libdispatch.dylib 0x00007fff6fa4bed1 _dispatch_continuation_pop + 472 49 libdispatch.dylib 0x00007fff6fa43783 _dispatch_async_redirect_invoke + 703 50 libdispatch.dylib 0x00007fff6fa3a9f9 _dispatch_root_queue_drain + 515 51 libdispatch.dylib 0x00007fff6fa3a7a5 _dispatch_worker_thread3 + 101 52 libsystem_pthread.dylib 0x00007fff6fd8a169 _pthread_wqthread + 1387 53 libsystem_pthread.dylib 0x00007fff6fd89be9 start_wqthread + 13

During the update feature development, add this to command line args to enable update feature. When update feature development is finished, update is only enabled in official build.

To change feed url, modify brave_feed_url in brave/brave_init_settings.gni

To see error log.

Update Brave item is enabled when --enable-brave-update-test command argument is used. This item is added for update test easily.

Second Info.plist tweaking is done by brave_tweak_info_plist("brave_app_plist") with fisrt tweaking result by tweak_info_plist("chrome_app_plist"). For this, //brave/build/mac/tweak_info_plist.[py|gni| is created.

With this, brave_app_plist target is added to //brave.

To build and run on chrome layer test targets, sparkle dependency should be included in them.

With this, we don't need to add any link-time dependency. BraveAppController requests update check to Sparkle via SparkleGlue.

RyanJarv

Approving for commit 326026a (approved by @riastradh-brave previously)

Fix shields isn't toggled when url has port number

Upgrade to Chromium 65.0.3325.146

simonhong requested a review from RyanJarv April 12, 2018 04:37

simonhong force-pushed the sparkle_integration branch from 25d64a4 to 29424bc Compare April 12, 2018 04:41

simonhong self-assigned this Apr 13, 2018

simonhong force-pushed the sparkle_integration branch 2 times, most recently from 5bd88c8 to 15419c5 Compare April 13, 2018 12:50

simonhong changed the title ~~WIP: Sparkle integration~~ WIP: Sparkle framework integration Apr 13, 2018

simonhong requested review from bbondy and darkdh April 13, 2018 15:08

RyanJarv reviewed Apr 14, 2018

View reviewed changes

RyanJarv mentioned this pull request Apr 14, 2018

Implement software updates brave/brave-browser#142

Closed

5 tasks

simonhong commented Apr 16, 2018

View reviewed changes

simonhong force-pushed the sparkle_integration branch from e3db07a to 2a34f0a Compare April 16, 2018 04:13

simonhong force-pushed the sparkle_integration branch 2 times, most recently from 40c409e to ce865d3 Compare April 16, 2018 12:55

simonhong force-pushed the sparkle_integration branch 2 times, most recently from cba171f to 0a23d87 Compare April 17, 2018 08:36

simonhong commented Apr 17, 2018

View reviewed changes

simonhong changed the title ~~WIP: Sparkle framework integration~~ Sparkle framework integration Apr 17, 2018

darkdh reviewed Apr 17, 2018

View reviewed changes

simonhong commented Apr 18, 2018

View reviewed changes

simonhong force-pushed the sparkle_integration branch from 5403f33 to 8767a88 Compare April 18, 2018 07:30

simonhong mentioned this pull request Apr 19, 2018

Integrate Sparkle to brave-core on MacOS brave/brave-browser#159

Closed

2 tasks

simonhong force-pushed the sparkle_integration branch from 8767a88 to a150250 Compare April 19, 2018 01:44

simonhong added 20 commits May 17, 2018 08:21

Add --enable-brave-update-test switch for update test

10a64a5

During the update feature development, add this to command line args to enable update feature. When update feature development is finished, update is only enabled in official build.

Set sparkle update url to Info.plist

e596b4f

To change feed url, modify brave_feed_url in brave/brave_init_settings.gni

Implement SUUpdaterDelegate protocol in BraveAppController

9e61c71

To see error log.

Add Update Brave item to main menu for update test

1d4263f

Update Brave item is enabled when --enable-brave-update-test command argument is used. This item is added for update test easily.

Cleanup - Make log more descriptive

0a544d6

Bundle pub key for DSA signatures and add sparkle to DEPS

cf04f01

Make sparkle build from its source

d5fea0d

Define public dsa file name to brave_init_settings.gni

d3c9a1b

Update sparkle repo revision

781933e

Cleanup gn configuration related with Sparkle framework integration

0d3241a

Second tweaking Info.plist is introduced

3b0ea2c

Second Info.plist tweaking is done by brave_tweak_info_plist("brave_app_plist") with fisrt tweaking result by tweak_info_plist("chrome_app_plist"). For this, //brave/build/mac/tweak_info_plist.[py|gni| is created.

Move chrome_app and chrome_app_plist target from //chrome to //brave

33a33bf

With this, brave_app_plist target is added to //brave.

Propagate sparkle dependency to test targets

71feac1

To build and run on chrome layer test targets, sparkle dependency should be included in them.

Use //chrome:chrome_app_plist instead of copying it to //brave

4fb017d

Use unified sparkle config and add some comment about sparkle

906d332

Use sparkle by using NSBundle

607ca79

With this, we don't need to add any link-time dependency. BraveAppController requests update check to Sparkle via SparkleGlue.

Rebase on Chromium 66

27dfdaa

Explicitly disable sparkle system profiling

5fcce88

Add comment to |brave_feed_url| and |kEnableBraveUpdateTest| switch

b843ef7

Use Sparkle framework of brave repo

326026a

simonhong dismissed riastradh-brave’s stale review via 326026a May 17, 2018 00:09

simonhong force-pushed the sparkle_integration branch from 2b92c6f to 326026a Compare May 17, 2018 00:09

RyanJarv approved these changes May 17, 2018

View reviewed changes

simonhong merged commit cd07fba into master May 17, 2018

simonhong deleted the sparkle_integration branch May 31, 2018 05:48

bbondy pushed a commit that referenced this pull request Feb 18, 2019

Merge pull request #91 from brave/fix_shields_with_port

d1a956a

Fix shields isn't toggled when url has port number

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #91 from brave/C65.0.3325.146

7ef873f

Upgrade to Chromium 65.0.3325.146

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sparkle framework integration #91

Sparkle framework integration #91

simonhong commented Apr 12, 2018 •

edited

Loading

RyanJarv commented Apr 14, 2018 •

edited

Loading

RyanJarv Apr 14, 2018 •

edited

Loading

simonhong Apr 16, 2018

simonhong left a comment •

edited

Loading

simonhong Apr 16, 2018

simonhong commented Apr 16, 2018 •

edited

Loading

RyanJarv commented Apr 16, 2018

RyanJarv commented Apr 17, 2018

bbondy commented Apr 17, 2018

simonhong commented Apr 17, 2018

RyanJarv commented Apr 17, 2018 •

edited

Loading

simonhong Apr 17, 2018

RyanJarv Apr 17, 2018

darkdh Apr 17, 2018

simonhong Apr 18, 2018

simonhong Apr 18, 2018

simonhong Apr 18, 2018 •

edited

Loading

RyanJarv left a comment •

edited

Loading

Sparkle framework integration #91

Sparkle framework integration #91

Conversation

simonhong commented Apr 12, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

RyanJarv commented Apr 14, 2018 • edited Loading

RyanJarv Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

simonhong Apr 16, 2018

Choose a reason for hiding this comment

simonhong left a comment • edited Loading

Choose a reason for hiding this comment

simonhong Apr 16, 2018

Choose a reason for hiding this comment

simonhong commented Apr 16, 2018 • edited Loading

RyanJarv commented Apr 16, 2018

RyanJarv commented Apr 17, 2018

bbondy commented Apr 17, 2018

simonhong commented Apr 17, 2018

RyanJarv commented Apr 17, 2018 • edited Loading

simonhong Apr 17, 2018

Choose a reason for hiding this comment

RyanJarv Apr 17, 2018

Choose a reason for hiding this comment

darkdh Apr 17, 2018

Choose a reason for hiding this comment

simonhong Apr 18, 2018

Choose a reason for hiding this comment

simonhong Apr 18, 2018

Choose a reason for hiding this comment

simonhong Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

RyanJarv left a comment • edited Loading

Choose a reason for hiding this comment

simonhong commented Apr 12, 2018 •

edited

Loading

RyanJarv commented Apr 14, 2018 •

edited

Loading

RyanJarv Apr 14, 2018 •

edited

Loading

simonhong left a comment •

edited

Loading

simonhong commented Apr 16, 2018 •

edited

Loading

RyanJarv commented Apr 17, 2018 •

edited

Loading

simonhong Apr 18, 2018 •

edited

Loading

RyanJarv left a comment •

edited

Loading