1110511 - Implement about:tabcrashed spec

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Our about:tabcrashed UX was spec'd out in bug 913651. We should actually implement that now.

Comment 1

[Tim Nguyen :ntim]

The styling landed in bug 1087618. What still needs to be done here is the functionality part (new buttons, crash reporter, ...).

Comment 2

[Dave Townsend [:mossop]]

Bug 1112304 implements most of the new functionality. The only remaining work here is to add the small UI block from http://people.mozilla.org/~mmaslaney/e10s/Firefox-e10s-v6.png which lets you add a comment and your email address to the crash report.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Ursula, are you planning on continuing to work on this?

Comment 4

[Ursula Sarracini (:ursula)]

Yup! I actually start on contract next week so I plan to clear this bug then.

Comment 5

[Ursula Sarracini (:ursula)]

Attachment: Implement about:tabcrashed spec

Comment 6

[Ursula Sarracini (:ursula)]

Comment on attachment 8665064 [details] [diff] [review]
Implement about:tabcrashed spec

mconley: WIP patch for this bug, UI is done but implementation for submitting a comment and emailing user is still in the works. Feel free to apply this patch and work off of it, or let me know what you want to do :)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'll try to drive this one through. Thanks for the start!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8665064 [details] [diff] [review]
Implement about:tabcrashed spec

Looks good! I'll drive this one through, thanks!

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - [WIP] Add comment and email input to about:tabcrashed.

Original patch by Ursula Sarracini

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - [WIP] Add comment and email input to about:tabcrashed.

Original patch by Ursula Sarracini

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r?felipe

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 13

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/20621/#review18611

Some small drive-by nits for the CSS.

::: browser/themes/shared/aboutTabCrashed.css:15
(Diff revision 3)
> +  background-color: rgb(235,235,235);

This corresponds to var(--in-content-box-background-hover).

::: browser/themes/shared/aboutTabCrashed.css:18
(Diff revision 3)
> +  border: 1px solid rgb(193,193,193);

The color corresponds to var(--in-content-box-border-color)

::: browser/themes/shared/aboutTabCrashed.css:19
(Diff revision 3)
> +  border-radius: 3px;

I'm not seeing any 3px border-radius in the common.inc.css file, only some 2px ones, it'd be better to be consistent here.

::: browser/themes/shared/aboutTabCrashed.css:28
(Diff revision 3)
> +textarea {

Please fix common.inc.css rather than hardcoding styles here.
The styles are supposed to be defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?from=common.inc.css#409 , but there was a typo while writing the code, html|textbox should have been html|input[type="text"] and html|textarea.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Ah, thanks ntim! Drive-by's welcome. :)

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

https://reviewboard.mozilla.org/r/20739/#review18803

Why the extraExtra?

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

https://reviewboard.mozilla.org/r/20739/#review18805

::: browser/modules/ContentCrashReporters.jsm:61
(Diff revision 1)
> -    if (CrashSubmit.submit(dumpID, { recordSubmission: true })) {
> +    let promise = CrashSubmit.submit(dumpID, {

Ah, I forgot to ask. Why did you remove the if block? And the promise result seems unused.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Hey, still WIP here - should have the final patches up tomorrow. :)

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

https://reviewboard.mozilla.org/r/20777/#review18807

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

https://reviewboard.mozilla.org/r/20779/#review18809

Comment 23

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/20619/#review18811

::: browser/base/content/browser.js:1166
(Diff revision 4)
> +          Comments: event.detail.comments,

Is the capital Comments/Email/URL here due to the format expected by the submission server?

::: browser/themes/shared/aboutTabCrashed.css:18
(Diff revision 4)
> +  border: 1px solid rgb(193,193,193);

are these background and border colors specific to the about:tabcrashed page or do you know if they can also be defined as some other --in-content var?

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:544
(Diff revision 4)
> +      dump("\nEt tu, Brute?\n");

I wonder if the dump() works here

Comment 24

[Ursula Sarracini (:ursula)]

https://reviewboard.mozilla.org/r/20619/#review18811

> are these background and border colors specific to the about:tabcrashed page or do you know if they can also be defined as some other --in-content var?

should be able to use ```border: 1px solid var(--in-content-border-color);``` from https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?from=common.inc.css#24

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r?felipe

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 29

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/20739/#review18805

> Ah, I forgot to ask. Why did you remove the if block? And the promise result seems unused.

Ah, good point about not doing anything with the promise. I'll remove that.

The thing is, CrashSubmit.submit never returns anything false-y, so the if-block didn't seem to be doing anything. Unless I'm missing something?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/20621/#review18849

::: browser/modules/ContentCrashReporters.jsm:20
(Diff revision 5)
> +  get defaults() {
> +    return {
> +      sendReport: true,
> +      includeURL: true,
> +      emailMe: true,
> +    }
> +  },

Not longer used

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r?felipe

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r?felipe

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r?felipe

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r?felipe

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe

Original patch by Ursula Sarracini

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 44

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Felipe Gomes from comment #23)
> https://reviewboard.mozilla.org/r/20619/#review18811
> 
> ::: browser/base/content/browser.js:1166
> (Diff revision 4)
> > +          Comments: event.detail.comments,
> 
> Is the capital Comments/Email/URL here due to the format expected by the
> submission server?

Yes, that's my understanding having read the socorro source - specifically this test: https://github.com/mozilla/socorro/blob/9b6c1ad54648594865b410bb15e9a32e54949207/socorro/unittest/processor/test_mozilla_transform_rules.py#L39

> 
> ::: browser/themes/shared/aboutTabCrashed.css:18
> (Diff revision 4)
> > +  border: 1px solid rgb(193,193,193);
> 
> are these background and border colors specific to the about:tabcrashed page
> or do you know if they can also be defined as some other --in-content var?
> 

Fixed!

> ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:544
> (Diff revision 4)
> > +      dump("\nEt tu, Brute?\n");
> 
> I wonder if the dump() works here

It does! :D

Comment 45

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/20621/#review18611

> Please fix common.inc.css rather than hardcoding styles here.
> The styles are supposed to be defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?from=common.inc.css#409 , but there was a typo while writing the code, html|textbox should have been html|input[type="text"] and html|textarea.

Hey - I don't think I fully understand this one. Are you saying I should be overwriting all of those instances of textbox with input[type="text"]? And adding textarea too?

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Thanks for the drive-by, Tim! Needinfo'ing for my question in comment 45.

Comment 47

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #45)
> https://reviewboard.mozilla.org/r/20621/#review18611
> 
> > Please fix common.inc.css rather than hardcoding styles here.
> > The styles are supposed to be defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?from=common.inc.css#409 , but there was a typo while writing the code, html|textbox should have been html|input[type="text"] and html|textarea.
> 
> Hey - I don't think I fully understand this one. Are you saying I should be
> overwriting all of those instances of textbox with input[type="text"]? And
> adding textarea too?

textbox is a XUL specific element. In the css, textbox is considered both as a XUL and HTML element which is incorrect. So basically, all html|textbox occurances need to be replaced with html|input[type="text"], html|textarea. The XUL occurance still needs to be kept for the xul consumers of the stylesheet. So the resulting code would be:

html|input[type="text"],
html|textarea,
xul|textbox {
  // default rules
}
html|input[type="text"]:focus,
html|textarea:focus,
xul|textbox[focused="true"] {
  ...
}

...and the same pattern applies for disabled.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tim Nguyen [:ntim] from comment #47)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #45)
> > https://reviewboard.mozilla.org/r/20621/#review18611
> > 
> > > Please fix common.inc.css rather than hardcoding styles here.
> > > The styles are supposed to be defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?from=common.inc.css#409 , but there was a typo while writing the code, html|textbox should have been html|input[type="text"] and html|textarea.
> > 
> > Hey - I don't think I fully understand this one. Are you saying I should be
> > overwriting all of those instances of textbox with input[type="text"]? And
> > adding textarea too?
> 
> textbox is a XUL specific element. In the css, textbox is considered both as
> a XUL and HTML element which is incorrect. So basically, all html|textbox
> occurances need to be replaced with html|input[type="text"], html|textarea.
> The XUL occurance still needs to be kept for the xul consumers of the
> stylesheet. So the resulting code would be:
> 
> html|input[type="text"],
> html|textarea,
> xul|textbox {
>   // default rules
> }
> html|input[type="text"]:focus,
> html|textarea:focus,
> xul|textbox[focused="true"] {
>   ...
> }
> 
> ...and the same pattern applies for disabled.

Ah, okay - I understand now. Thank you!

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r?ntim

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe,ntim

Original patch by Ursula Sarracini

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r?felipe

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r?felipe

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Hey ntim - for "Add comment and email input to about:tabcrashed.", I'm hoping you can give the changes I made to the in-content common.css a look. Thanks!

Comment 54

[Tim Nguyen :ntim]

Comment on attachment 8668585 [details]
MozReview Request: Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

https://reviewboard.mozilla.org/r/20993/#review18875

Looks good to me!

Comment 55

[Tim Nguyen :ntim]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

https://reviewboard.mozilla.org/r/20621/#review18877

::: toolkit/themes/shared/in-content/common.inc.css:424
(Diff revision 8)
> +  height: 30px;

While you're changing this, can you remove the hardcoded line-height, and change the height to a min-height like we currently do with buttons (dao originally suggested the change for buttons) ? These rules are not HDPI friendly.

::: toolkit/themes/shared/in-content/common.inc.css:434
(Diff revision 8)
> +  padding: 10px;

I haven't checked how it looks, but please make sure the height is (approximately) 30px on 1x displays, simply for consistency with other in-content UI textboxes.

Comment 56

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #55)
> Comment on attachment 8666904 [details]
> ::: toolkit/themes/shared/in-content/common.inc.css:434
> (Diff revision 8)
> > +  padding: 10px;
> 
> I haven't checked how it looks, but please make sure the height is
> (approximately) 30px on 1x displays, simply for consistency with other
> in-content UI textboxes.

This comment of course doesn't apply to text areas, just single line text inputs.

Comment 57

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

https://reviewboard.mozilla.org/r/20739/#review18883

::: browser/modules/ContentCrashReporters.jsm:61
(Diff revisions 3 - 4)
> -    let promise = CrashSubmit.submit(dumpID, {
> +    CrashSubmit.submit(dumpID, {

this will probably generate a warning for a uncaught promise. It would be nice to do something in the UI to better handle this (maybe only remove the controls after success?), but for now let's at least add .then(null, Cu.reportError) here

Comment 58

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8668558 [details]
MozReview Request: Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

https://reviewboard.mozilla.org/r/20975/#review18885

Comment 59

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/20621/#review18877

> I haven't checked how it looks, but please make sure the height is (approximately) 30px on 1x displays, simply for consistency with other in-content UI textboxes.

Alright - I've altered the margins so that it matches the textboxes in the Preferences dialog.

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8668558 [details]
MozReview Request: Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8668585 [details]
MozReview Request: Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r?felipe,ntim

Original patch by Ursula Sarracini

Comment 64

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r=felipe

Comment 66

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

https://reviewboard.mozilla.org/r/20621/#review19191

::: browser/modules/ContentCrashReporters.jsm:103
(Diff revision 9)
> +    this.prefs.setCharPref("email", aFormData.email);

as talked on irc, let's clear the email pref in case emailMe is false

Comment 67

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8668558 [details]
MozReview Request: Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667371 [details]
MozReview Request: Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8668585 [details]
MozReview Request: Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666904 [details]
MozReview Request: Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

Original patch by Ursula Sarracini

Comment 71

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667574 [details]
MozReview Request: Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

Comment 72

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8667575 [details]
MozReview Request: Bug 1110511 - Regression tests. r=felipe

Bug 1110511 - Regression tests. r=felipe

Comment 73

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/0e055b648e1003d29b243e036aef43565f2e9874
Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/219d86f78b6be95ffbac8d005c428cd04db11150
Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/c7b983f655685051c0ed691ca1dfbdd47a3e964a
Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

https://hg.mozilla.org/integration/fx-team/rev/8e20cd68ca78366976495c9a7f003f0f120c1166
Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

https://hg.mozilla.org/integration/fx-team/rev/0eb0dda094b30354a84991e430a0e49320fde52b
Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

https://hg.mozilla.org/integration/fx-team/rev/eddfd7f32a1cd0a7972ce2c459d53c7b750ec9a7
Bug 1110511 - Regression tests. r=felipe

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4fb5f812f7c9 for mochitest-bc timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=4990027&repo=fx-team


15:23:21 INFO - JavaScript error: resource://testing-common/BrowserTestUtils.jsm, line 25: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The failures so far have all been on Linux ASAN builds, not sure if that'll continue or just a coincidence.

Comment 76

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/c6fe1019a8394c4b3181bacbccec04c0e5523d0c
Bug 1110511 - Use the browser permanentKey to map to childIDs in TabCrashReporter. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/f6cb65396893532ca142eb873e37ef21cf3a15a5
Bug 1110511 - Allow TabCrashReporter to pass extraExtraKeyVals. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/d14961fccedfe7c670ab5f4eff204b16a408a993
Bug 1110511 - Fix erroneous references to html|textbox in in-content's common.css. r=ntim

https://hg.mozilla.org/integration/fx-team/rev/717c687684d3ea30fe05c10d205bedadede768b3
Bug 1110511 - Add comment and email input to about:tabcrashed. r=felipe,ntim

https://hg.mozilla.org/integration/fx-team/rev/9758dfd459cb23cda365c2a7eb05887000c726d0
Bug 1110511 - Move tab-crashing test helper function to BrowserTestUtils.jsm r=felipe

https://hg.mozilla.org/integration/fx-team/rev/d37602b95f4445605a45be788305b00865666264
Bug 1110511 - Regression tests. r=felipe

Comment 77

[Mike Conley (:mconley) (:⚙️)]

It looks like KeyValueParser.jsm isn't available unless MOZ_CRASHREPORTER is set to true. I've modified BrowserTestUtils.jsm in 9758dfd459 to account for this.

Here's the try push, with the change on tip:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e51e4b694c

Comment 78

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6fe1019a839
https://hg.mozilla.org/mozilla-central/rev/f6cb65396893
https://hg.mozilla.org/mozilla-central/rev/d14961fccedf
https://hg.mozilla.org/mozilla-central/rev/717c687684d3
https://hg.mozilla.org/mozilla-central/rev/9758dfd459cb
https://hg.mozilla.org/mozilla-central/rev/d37602b95f44

Comment 79

[Paul Silaghi, QA [:pauly]]

I tested this in latest nightly 46.0a1 (2016-01-18) on Win 7.
A few issues:
1. The crash reporter is not expanded if was automatically checked on crash http://i.imgur.com/szmLXNf.png - unchecking/checking again reveals the crash reporter
2. other tab icons disappear when a tab crashes http://i.imgur.com/x8upQ7f.png
3. I don't see the report confirmation message (last picture from the mockup - "crash report already submitted etc")
4. unable to use the keyboard for checking the "submit report" option (also its sub-boxes)
5. able to enter email address even if "email me when more info is available" option is unchecked
6. when a second tab crashes, "restore all crashed tabs" option appears in both tabs. But after I close the second tab, "restore all crashed tabs" option remains displayed in the first tab, even if there is only 1 tab crashed
Let me know your thoughts and if any of them should be filed separately. Thanks.

Comment 80

[Mike Conley (:mconley) (:⚙️)]

Hey pauly - thanks for poking at this. You've found some very good stuff here.

(2) is known - that's bug 1227630

The rest are new and should be separately filed, and have tracking-e10s set to ?.

Are you good to file those, or did you want me to do it?

Comment 81

[Paul Silaghi, QA [:pauly]]

I'll file them tomorrow. Thanks.

Comment 82

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi, QA [:pauly] from comment #79)
> I tested this in latest nightly 46.0a1 (2016-01-18) on Win 7.
Since this bug appears fixed on 44, I've re-tested the scenarios on FF 44b9 and got different results:
1. not reproducible
2. not reproducible - crashing a tab, makes all the other tabs crash too (not sure if expected)
3. not reproducible
4. reproducible
5. reproducible
6. reproducible
This means 1-3 are regressions I guess...
Thoughts?

Comment 83

[Mike Conley (:mconley) (:⚙️)]

Hey pauly, it's likely fallout from bug 1220929, which refactored how we handle crashed tabs.

Comment 84

[Paul Silaghi, QA [:pauly]]

Ok then, I'm marking this bug as verified on FF 44, considering the follow-up bugs.

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1110511.html