EndPageLoad, DisplayLoadError fail silently on unhandled errors

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: geko1702+bugzilla, Assigned: geko1702+bugzilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
We should have catch-all assertions hinting that specific error handling must be added.
(Assignee)

Comment 1

4 years ago
Created attachment 8456558 [details] [diff] [review]
added MOZ_ASSERTION statements to point out missing error handling
(Assignee)

Updated

4 years ago
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8457649 [details] [diff] [review]
added NS_WARNING, MOZ_ASSERTION statements to point out missing error handling
(Assignee)

Updated

4 years ago
Attachment #8456558 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Apparently we already have unhandled errors (e.g., NS_BINDING_ABORTED) so we should use a warning instead of an assertion in EndPageLoad(). DisplayLoadError() should be more rigid though so we should stick with the assertion there.

https://tbpl.mozilla.org/?tree=Try&rev=12f7864c1899
(Assignee)

Comment 6

4 years ago
Comment on attachment 8457649 [details] [diff] [review]
added NS_WARNING, MOZ_ASSERTION statements to point out missing error handling

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

I think it's a bad idea to have unhandled cases where the docshell fails to load. If the docshell represents the topmost frame then the tab doesn't seem to go past its initial page (e.g., tiles screen) and there's no visual indication of the failure -- it just looks frozen. Printing a warning will help developers realize that they are missing something when introducing a new reason to fail.
Attachment #8457649 - Flags: review?(bugs)

Comment 7

4 years ago
Comment on attachment 8457649 [details] [diff] [review]
added NS_WARNING, MOZ_ASSERTION statements to point out missing error handling

>         default:
>+            // Should always display something.
>+            MOZ_ASSERT(false, "Unknown load error.");
>             break;
>         }
>     }
> 
>     // Test if the error should be displayed
>     if (error.IsEmpty()) {
>         return NS_OK;
>     }

error.IsEmpty() explicitly handles the case when we're not about to show error.
So MOZ_ASSERT doesn't really make sense.
Either we have MOZ_ASSERT or if (error.IsEmpty()), not both.



>+        // Shouldn't reach this place, i.e., NS_FAILED with no error handler
>+        // will have the frame silently fail.
>+        else {
>+            NS_WARNING(nsPrintfCString("Page load ended with unhandled error 0x%x (severity: %d, module: %d, code: %d)", aStatus,
>+                NS_ERROR_GET_SEVERITY(aStatus), NS_ERROR_GET_MODULE(aStatus),
>+                NS_ERROR_GET_CODE(aStatus)).get());

Didn't you say we enter this code with NS_BINDING_ABORTED. Let's not add warning about that case.
Attachment #8457649 - Flags: review?(bugs) → review-
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.