1380119 - Update Screenshots add-on to 10.4

Status: RESOLVED INVALID

(Firefox :: Screenshots, enhancement)

Description

[John Gruen [:jgruen]]

Changlog pending, while :ianbicking cherry picks commits onto 10.3. 

This branch consists of three patches on top of 10.3 (currently in Beta, CF 1372310)

The first patch adds an icon signaling that saved shots get uploaded:
https://github.com/mozilla-services/screenshots/pull/3108

The second adds some sanitizing checks to downloaded shot names:
https://github.com/mozilla-services/screenshots/pull/3084

And the third adds dark theme support for the icons:
https://github.com/mozilla-services/screenshots/pull/3109

None of these strike me as risky; however the third patch is dependent on two bugzilla bugs also getting uplift...these are:
https://bugzilla.mozilla.org/show_bug.cgi?id=1379464
https://bugzilla.mozilla.org/show_bug.cgi?id=1377302

Should these blocking bugs not be eligible for Beta, we can cut a version of 10.4 with not-so-awesome-but-passible version of 3109 with a drop shadow on the icon.

Comment 2

[John Gruen [:jgruen]]

:ritu recommended we request uplift on everything tomorrow morning for beta 9 which gets tagged Thursday. Ritu, can you confirm that?

Comment 3

[Ian Bicking (:ianbicking)]

Attachment: Bug 1380119 - Export Screenshots 10.4.0 to Firefox;

Review commit: https://reviewboard.mozilla.org/r/156288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156288/

Comment 4

[Ian Bicking (:ianbicking)]

Changelog: https://github.com/mozilla-services/screenshots/blob/1bcbddaa4f805ce7237606d0628dbc2e87cae7f4/CHANGELOG.md#version-1040

- Add context fill icons https://github.com/mozilla-services/screenshots/commit/7cb237f

- Sanitize download filename more fully. This adds : (important on Windows), \, <, and > to the blacklist. Followup in https://github.com/mozilla-services/screenshots/issues/3083 Fixes https://github.com/mozilla-services/screenshots/issues/2981 https://github.com/mozilla-services/screenshots/commit/af32978

- Add cloud icon to Save https://github.com/mozilla-services/screenshots/commit/4ae42cc

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This doesn't meet the review requirements in MozReview for Autoland to push it.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8885437 [details]
Bug 1380119 - Export Screenshots 10.4.0 to Firefox;

https://reviewboard.mozilla.org/r/156288/#review161784

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73ffdc1b8c09
Export Screenshots 10.4.0 to Firefox; r=mossop

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to [:jgruen] from comment #2)
> :ritu recommended we request uplift on everything tomorrow morning for beta
> 9 which gets tagged Thursday. Ritu, can you confirm that?

If the fixes are ready and stable, I'd recommend uplifting asap. We gtb b9 Thursday ~10am PST. If this is not ready then, the next chance to push this would be with b10 that goes to Beta channel on Tuesday.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73ffdc1b8c09

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

this code is causing talos ts_paint to fail often with a crash:
https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win32/1499895920/autoland_win7_ix_test-other-e10s-bm111-tests1-windows-build271.txt.gz

and this data:
09:37:36     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/UAvlg_3-THWSpMqfWx5Iww/artifacts/public/build/firefox-56.0a1.en-US.win32.crashreporter-symbols.zip
09:37:44     INFO -  mozcrash Copy/paste: C:\slave\test\build\win32-minidump_stackwalk.exe c:\users\cltbld~1.000\appdata\local\temp\tmp2kxx32\profile\minidumps\18521cc5-e553-4c12-83be-eb36c73b0249-browser.dmp c:\users\cltbld~1.000\appdata\local\temp\tmplv3awz
09:37:52     INFO -  mozcrash Saved minidump as C:\slave\test\build\blobber_upload_dir\18521cc5-e553-4c12-83be-eb36c73b0249-browser.dmp
09:37:52     INFO -  PROCESS-CRASH | ts_paint | application crashed [@ google_breakpad::ExceptionHandler::WriteMinidump(std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const &,bool (*)(wchar_t const *,wchar_t const *,void *,_EXCEPTION_POINTERS *,MDRawAssertionInfo *,bool),void *,_MINIDUMP_TYPE)]
09:37:52     INFO -  Crash dump filename: c:\users\cltbld~1.000\appdata\local\temp\tmp2kxx32\profile\minidumps\18521cc5-e553-4c12-83be-eb36c73b0249-browser.dmp
09:37:52     INFO -  Operating system: Windows NT
09:37:52     INFO -                    6.1.7601 Service Pack 1
09:37:52     INFO -  CPU: x86
09:37:52     INFO -       GenuineIntel family 6 model 30 stepping 5
09:37:52     INFO -       8 CPUs
09:37:52     INFO -  GPU: UNKNOWN
09:37:52     INFO -  Crash reason:  EXCEPTION_NONCONTINUABLE_EXCEPTION
09:37:52     INFO -  Crash address: 0x0
09:37:52     INFO -  Process uptime: 3 seconds
09:37:52     INFO -  Thread 48 (crashed)
09:37:52     INFO -   0  xul.dll!google_breakpad::ExceptionHandler::WriteMinidump(std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const &,bool (*)(wchar_t const *,wchar_t const *,void *,_EXCEPTION_POINTERS *,MDRawAssertionInfo *,bool),void *,_MINIDUMP_TYPE) [exception_handler.cc:73ffdc1b8c09 : 764 + 0xb]
09:37:52     INFO -      eip = 0x6530da0d   esp = 0x1a9dfa44   ebp = 0x1a9dfb18   ebx = 0x1a356570
09:37:52     INFO -      esi = 0x1a9dfa48   edi = 0x00000000   eax = 0x1a9df71c   ecx = 0x00000008
09:37:52     INFO -      edx = 0x00000000   efl = 0x00000212
09:37:52     INFO -      Found by: given as instruction pointer in context
09:37:52     INFO -   1  xul.dll!CrashReporter::CreateMinidumpsAndPairInternal(void *,unsigned long,nsCString,nsCOMPtr<nsIFile>,nsIFile * *,std::function<void > &&,RefPtr<nsIThread> &&,bool) [nsExceptionHandler.cpp:73ffdc1b8c09 : 4118 + 0x1b]
09:37:52     INFO -      eip = 0x65305a5f   esp = 0x1a9dfb20   ebp = 0x1a9dfbd0
09:37:52     INFO -      Found by: call frame info
09:37:52     INFO -   2  xul.dll!std::_Invoker_functor::_Call<<lambda_cdbdd5506a14d758a2b5725521de2b01> &>(<lambda_cdbdd5506a14d758a2b5725521de2b01> &) [type_traits:73ffdc1b8c09 : 1375 + 0x3d]
09:37:52     INFO -      eip = 0x653049f6   esp = 0x1a9dfbd8   ebp = 0x1a9dfc04
09:37:52     INFO -      Found by: call frame info
09:37:52     INFO -   3  xul.dll!std::_Func_impl<<lambda_cdbdd5506a14d758a2b5725521de2b01>,std::allocator<int>,void>::_Do_call() [functional:73ffdc1b8c09 : 212 + 0x9]
09:37:52     INFO -      eip = 0x6530a8b3   esp = 0x1a9dfc0c   ebp = 0x1a9dfc14
09:37:52     INFO -      Found by: call frame info
09:37:52     INFO -   4  xul.dll!mozilla::detail::RunnableFunction<std::function<void > >::Run() [nsThreadUtils.h:73ffdc1b8c09 : 507 + 0x15]
09:37:52     INFO -      eip = 0x653089b2   esp = 0x1a9dfc14   ebp = 0x1a9dfc14
09:37:52     INFO -      Found by: call frame info
09:37:52     INFO -   5  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:73ffdc1b8c09 : 1437 + 0x6]
09:37:52     INFO -      eip = 0x635b07c1   esp = 0x1a9dfc1c   ebp = 0x1a9dfd80
09:37:52     INFO -      Found by: call frame info


I recommend backing this out so we can continue to get performance data.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out as requested by Mossop and jmaher for frequently crashing talos ts_paint:

https://hg.mozilla.org/mozilla-central/rev/5c3e7cb5a35e1e56c7eb24ca392d22309f7af9f5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=73ffdc1b8c09e0ca55050703d96f06209205f3cb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 12

[Wil Clouser [:clouserw]]

Backing out sounds reasonable for crashes.

I notice the crash is on win32 and OOP just landed 3 days ago in bug 1357486.  Andy -- do you think these are related?

If Talos keeps crashing after this is backed out, that could point us in the right direction.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

as a note, this had a small performance regression:
== Change summary for alert #7914 (as of July 12 2017 21:45 UTC) ==

Regressions:

  2%  cart summary linux64 opt e10s     9.95 -> 10.16

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7914


on the backout I assume we will see a corresponding improvement.

Comment 14

[Ian Bicking (:ianbicking)]

We think this may have happened because my export unintentionally backed out the fix to Bug 1380616 – for some reason I kept reading that part of the diff incorrectly. I'm planning on making a 10.5.0 export that fixes that problem, and adds one other small fix we've identified.

Comment 15

[Ian Bicking (:ianbicking)]

I'm trying this again with a 10.5.0 version in Bug 1380817

Comment 16

[Wil Clouser [:clouserw]]

Thanks for helping track this down everyone