1039045 - EndPageLoad, DisplayLoadError fail silently on unhandled errors

Status: RESOLVED WONTFIX

(Core :: DOM: Navigation, defect)

Description

[Georgios Kontaxis]

We should have catch-all assertions hinting that specific error handling must be added.

Comment 1

[Georgios Kontaxis]

Attachment: added MOZ_ASSERTION statements to point out missing error handling

Comment 2

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=abb11353d5b5

Comment 3

[Georgios Kontaxis]

Attachment: added NS_WARNING, MOZ_ASSERTION statements to point out missing error handling

Comment 4

[Georgios Kontaxis]

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

Comment 5

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=29688b197d9c

Comment 6

[Georgios Kontaxis]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.