Patch make_incremental_update.sh to only include forced updates if they differ?
I read through make_incremental_update.sh a bit closer while cleaning up my patch for bug13375, and I am concerned that $requested_forced_updates and the associated check_for_forced_update() is going to cause a lot of unnecessary load on the Tor network due to including full copies of all of our addons regardless of if they change.
Can we make check_for_forced_update() only include full copies of stuff from the forced_update line if it has actually changed between releases? I notice that the original code also forced Contents/MacOS/TorBrowser.app/Contents/MacOS/firefox. I'm not sure if we should continue to treat that as full copies regardless of change. Possibly?
Is there substantial risk for trying to do this change for 4.5a5? The change itself seems simple enough to me (just use the diff check from the next block in a conjunction).
Activity
I do not think it is a good idea to make this change without testing and considering what might break. The main reason that Kathy and I set things up to always force a full replace of the bundled add-ons was to reduce load on the Tor network by making it more likely that an incremental update would succeed. If we do not "force replace" the add-ons then, if an add-on is updated by the user, the next incremental update will fail. I fear that might be fairly common, which means the update service will be more likely to fallback to a full update.
mcs - I am asking a different question than what I asked last week on IRC. From my read of check_for_forced_update(), it seems we always include copies of all of our addons. So, even though HTTPS-Everywhere, torbutton, and tor-launcher did not change in 4.0.5, it appears as though that function will still cause us to include a full copy of them. This seems wrong.
Is there some reason that the MAR update is checking checksums even of files we don't try to update? If so, that seems like a good reason to hold off on this, but it also seems like something we should fix.
Independently, it also seems like torbutton and tor-launcher should not even be in this list, because we set them to be unable to update already (because we set both of their update urls to localhost, to prevent rogue versions from being listed on addons.mozilla.org and being downloaded).
-
Replying to mikeperry:
mcs - I am asking a different question than what I asked last week on IRC. From my read of check_for_forced_update(), it seems we always include copies of all of our addons. So, even though HTTPS-Everywhere, torbutton, and tor-launcher did not change in 4.0.5, it appears as though that function will still cause us to include a full copy of them. This seems wrong.
Is there some reason that the MAR update is checking checksums even of files we don't try to update? If so, that seems like a good reason to hold off on this, but it also seems like something we should fix.
I understand your point now. I am fairly sure that only the files that are mentioned in the incremenal MAR are examined while applying an update. So, yes, we could avoid force-update for add-ons that have not changed between releases. Kathy and I will look at make_incremental_update.sh and see how easy or difficult it will be to make that change. For HTTPS-Everywhere it may be tricky because that extension is unzipped (many files) and the incremental MAR creation algorithm tends to operate file-by-file.
Independently, it also seems like torbutton and tor-launcher should not even be in this list, because we set them to be unable to update already (because we set both of their update urls to localhost, to prevent rogue versions from being listed on addons.mozilla.org and being downloaded).
Good point. That change could be made immediatately.
-
We also have pdf.js in the list as a separate extension:
exts='https-everywhere@eff.org/* tor-launcher@torproject.org.xpi torbutton@torproject.org.xpi uriloader@pdf.js.xpi {73a6fe31-595d-460b-a920-fcc0f8843232}.xpi'I am not sure if we ever shipped that as a bundled extension within the TB browser profile, but I am pretty sure pdf.js is built into TB 4.5... so I think we can get rid of that entry as well as the Tor Launcher and Torbutton entries.
-
For the record, in the dev meeting, we discussed that the ideal situation would be to update all files in the https-everywhere directory if any files in that directory either differ, were added, or were removed. We should ensure both adding and removing files are supported by the force update, though I expect that if any files were added or removed, the install.rdf will always differ because the version in it will differ. This means the check can probably be simple, but we will need to special case this addon, and update all files in the directory in tandem.
NoScript is easier to handle, as we only need to force update the whole noscript xpi if it differs.
-
Kathy and I have a patch that addresses the low hanging fruit. Please review:
Next, we will work on the more complicated fix (i.e., check to see if NoScript or HTTPS Everywhere has really changed).
Trac:
Keywords: TorBrowserTeam201503 deleted, TorBrowserTeam201503R added
Status: new to needs_review -
And here is the other part of the fix:
Trac:
Status: needs_review to assigned
Owner: tbb-team to mcs -
Ok, this looks good. For the record, I decided that we should try to get some analytics before trying to modify this to also handle the startup scripts, so we don't risk delaying the release. I created #15456 (moved) to remind us to check the webserver logs in April to determine if it is worthwhile to even do this type of hack for the startup scripts.
Thanks!
Trac:
Status: needs_review to closed
Resolution: N/A to fixed
