Fix issues in test pilot validation

RESOLVED FIXED in Thunderbird 13.0

Status

Thunderbird
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: standard8, Assigned: Irving)

Tracking

Trunk
Thunderbird 13.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird12 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 601553 [details]
Extracted Validation Log

Various issues were found in the AMO validation of the test pilot xpi - see attached files.

We need to fix these asap so we can start pushing out studies.

For the innerHTML stuff, see also https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion
Based on the discussion linked, I think we should push back on the evalInSandbox warning.

https://forums.mozilla.org/addons/viewtopic.php?f=14&t=4532&view=previous&sid=fb4b5f292960eda5d89a2198a8df5dd4
Status: NEW → ASSIGNED
(Reporter)

Comment 2

6 years ago
(In reply to Irving Reid (:irving) from comment #1)
> Based on the discussion linked, I think we should push back on the
> evalInSandbox warning.
> 
> https://forums.mozilla.org/addons/viewtopic.
> php?f=14&t=4532&view=previous&sid=fb4b5f292960eda5d89a2198a8df5dd4

Agreed.
Created attachment 601945 [details] [diff] [review]
Work in progress, needs testing

Mostly replaced assignments to innerHTML with textContent, except one place where I use the HTML sanitizer service - this needs to be tested.
Attachment #601945 - Flags: feedback?(mbanner)
(Reporter)

Comment 4

6 years ago
Comment on attachment 601945 [details] [diff] [review]
Work in progress, needs testing

This looks fine. I've been using it for the test we've got running at the moment and I've not noticed any issues.

We've not tested the survey generator, but we can always hook that up later.

The AMO validator output looks a lot better as well.

So I think this looks fine, but I'm going to see if Gregg can give us some feedback as well today before we land it.
Attachment #601945 - Flags: review+
Attachment #601945 - Flags: feedback?(mbanner)
Attachment #601945 - Flags: feedback?(glind)
Comment on attachment 601945 [details] [diff] [review]
Work in progress, needs testing

Looking back, I think I used appendElement() in a few places where I probably should have used appendChild() to add elements to the HTML content - I'm not sure whether these are interchangeable.
OK, now I've seen an error message in my stderr that needs to be tracked down:

JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert JavaScript argument arg 1 [nsIDOMEventTarget.addEventListener]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: chrome://testpilot/content/all-studies-window.js :: <TOP_LEVEL> :: line 190"  data: no]

Looking into it...
STR the addEventListener error: Start up TB with the patched test pilot, go to Tools/Test Pilot/All Your User Studies

Good thing the assertion message didn't give me any information about the JS object it was trying to convert, he says snarkily. An added dump tells me the inputs to addButton are:

"Submit", "submit-button-10002", "TestPilotXulWindow.onSubmitButton(10002);"

Converting the Javascript string argument into an anonymous function declaration makes this error go away, now testing further to make sure it actually works...
Created attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

changed appendElement() to appendChild() and fixed the error in addEventListener. Still needs testing, particularly the survey_generator.js changes.
Attachment #601945 - Attachment is obsolete: true
Attachment #603030 - Flags: feedback?(mbanner)
Attachment #601945 - Flags: feedback?(glind)
(Reporter)

Comment 9

6 years ago
Comment on attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

Review of attachment 603030 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on irc, I've been running this for the last few days and not found any issues.

Although survey-generator.js may need a few more tweaks, we'll be testing any surveys before pushing them out, so I think we're all-right to go with it as-is.

Hence r=me.
Attachment #603030 - Flags: feedback?(mbanner) → review+
(Reporter)

Comment 10

6 years ago
Checked in: 

http://hg.mozilla.org/comm-central/rev/66fd8a08a63b

I also did a version bump for test pilot so that we can push these to AMO:

http://hg.mozilla.org/comm-central/rev/6a007b23cc21
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(Reporter)

Updated

6 years ago
Blocks: 736984
(Reporter)

Comment 11

6 years ago
Comment on attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

[Triage Comment]
Taking to beta as I want to get the version of test pilot we're distributing, synced up with the version on AMO.
Attachment #603030 - Flags: approval-comm-beta+
(Reporter)

Comment 12

6 years ago
Checked in:

http://hg.mozilla.org/releases/comm-beta/rev/9502ae8490b0
status-thunderbird13: --- → fixed
Blocks: 737892
(Reporter)

Updated

6 years ago
status-thunderbird12: --- → fixed
status-thunderbird13: fixed → ---
You need to log in before you can comment on or make changes to this bug.