Closed Bug 1057497 Opened 7 years ago Closed 7 years ago

BrowserElementChildPreload.js shouldn't duplicate the categorization of NSS/certificate errors provided by nsINSSErrorsService

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: keeler, Assigned: Cykesiopka)

Details

Attachments

(1 file)

getErrorClass in BrowserElementChildPreload.js duplicates the functionality of nsINSSErrorsService.getErrorClass. Keeping the two implementations in sync is error-prone and cumbersome. Another solution should be found if it can't call nsINSSErrorsService.getErorrClass directly.
Just a quick glance, I think we could use nsINSSErrorsService.getErorrClass directly.
Just attaching this here for now.

I did a bit of manual testing on a B2G Desktop build, and this seems to work.

However:
 - I don't know if automated tests cover this path (test_cert_overrides.js maybe?)
 - I need to find out if B2G is missing a "Secure Connection Failed" page, or if something else is going on
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Comment on attachment 8506633 [details] [diff] [review]
bug1057497_v1.patch

(In reply to Cykesiopka from comment #2)
>  - I need to find out if B2G is missing a "Secure Connection Failed" page,
> or if something else is going on

It looks like this is a separate issue.

I did more manual testing with different error codes, and from staring at debug DOMWINDOW output, B2G is at least trying to load about:certerror and about:neterror for the appropriate errors.
Attachment #8506633 - Flags: review?(kchen)
Comment on attachment 8506633 [details] [diff] [review]
bug1057497_v1.patch

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

Looks good.

browserElement_ErrorSecurity.js is the test for this part.
Attachment #8506633 - Flags: review?(kchen) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #4)
> browserElement_ErrorSecurity.js is the test for this part.

Thanks!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f242eedce11e
Keywords: checkin-needed
(Removing checkin-needed because I didn't look carefully at what the browserElement_ErrorSecurity.js tests run on - apparently not B2G Desktop Linux x64 opt as I expected.)
Keywords: checkin-needed
Now with actual test results on the code path I'm changing to complement the previous try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee23eac30ef3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb856489ba05
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.