1006379 - JS error in language detection code when closing a tab that's still loading

Status: VERIFIED FIXED

(Firefox :: Translation, defect)

Description

[Florian Quèze [:florian] (on leave until October 9)]

[Exception... "[JavaScript Error: "content is null" {file: "chrome://browser/content/content.js" line: 413}]'[JavaScript Error: "content is null" {file: "chrome://browser/content/content.js" line: 413}]' when calling method: [nsIWebProgressListener::onStateChange]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/tabbrowser.xml :: _endRemoveTab :: line 2071"  data: yes]

Line 413 is:
    encoder.init(content.document, "text/plain", encoder.SkipInvisibleContent);

I noticed this JS error in my terminal when I closed a tab that was still loading.

I guess it would be trivial to just add a null check on "content" and return early if it's null, but looking at the values of the parameters given to the onStateChange method may be a better solution.

Comment 1

[Florian Quèze [:florian] (on leave until October 9)]

Mass move of translation bugs to the new Translation component.

Comment 2

[:Gijs (he/him)]

Attachment: don't attempt language detection on aborted page loads,

AFAICT the only information we have is if the load aborted. So we can do this, but it'd break translation of pages where the user hits stop/escape or aborts the load for some other reason.

Comment 3

[:Gijs (he/him)]

Attachment: don't attempt language detection on aborted page loads,

Ergh, turns out Cr isn't defined yet. Fixing that.

Comment 4

[Florian Quèze [:florian] (on leave until October 9)]

Comment on attachment 8423100 [details] [diff] [review]
don't attempt language detection on aborted page loads,

This attachment will conflict with attachment 8420802 [details] [diff] [review].

Comment 5

[:Gijs (he/him)]

Attachment: don't attempt language detection if the document is gone,

Alternatively, we could just nullcheck.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8423123 [details] [diff] [review]
don't attempt language detection if the document is gone,

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

I think detecting the language when the user has pressed Stop is a valid use case, so let's go with this option..

Do we get the same NS_BINDING_ABORTED for both a user-triggered stop and for a document-is-gone stop?

Comment 7

[:Gijs (he/him)]

(In reply to :Felipe Gomes from comment #6)
> Comment on attachment 8423123 [details] [diff] [review]
> don't attempt language detection if the document is gone,
> 
> Review of attachment 8423123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think detecting the language when the user has pressed Stop is a valid use
> case, so let's go with this option..
> 
> Do we get the same NS_BINDING_ABORTED for both a user-triggered stop and for
> a document-is-gone stop?

Yes, AFAICT there's no way to distinguish the two cases from the web progress listener's pov. :-\

(I did also test and the other patch does indeed break the "press stop during page load, then translate" usecase)

Comment 8

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/d2214e4edf1b

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2214e4edf1b