1186346 - JavaScript error: chrome://browser/content/tab-content.js, line 289: TypeError: content is null

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Jesse Ruderman]

Attachment: testcase

JavaScript error: chrome://browser/content/tab-content.js, line 289: TypeError: content is null

This code was added in https://hg.mozilla.org/mozilla-central/rev/ad3867772421:

  get isAboutReader() {
    return content.document.documentURI.startsWith("about:reader");
  },

Comment 1

[:Margaret Leibovic]

I think it would be safe enough to add a null check here and just return false if content is null.

To fix this bug, you need to update the code here:
http://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/browser/base/content/tab-content.js#l288

Comment 2

[Giovanny Gongora [:gioyik]]

Attachment: 1186346.patch

Comment 3

[Giovanny Gongora [:gioyik]]

Hi Margaret,

I created a patch for this one, let me know if the null check is correct and the patch is a good fix for this.

Thanks

Comment 4

[:Margaret Leibovic]

Comment on attachment 8638247 [details] [diff] [review]
1186346.patch

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

Thanks for the patch! This looks reasonable to me, but I'd like a second opinion from a /browser peer, since I'm mainly a mobile developer :)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8638247 [details] [diff] [review]
1186346.patch

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

::: browser/base/content/tab-content.js
@@ +287,5 @@
>  
>    get isAboutReader() {
> +    if (content == null) {
> +      return false;
> +    }

This should just be:
if (!content) {
  return false;
}

Comment 6

[Giovanny Gongora [:gioyik]]

I forgot this issue due I was not assigned and didn't appear in my dashboard. Attaching a new patch.

Comment 7

[Giovanny Gongora [:gioyik]]

Attachment: 1186346.patch

Comment 8

[Giovanny Gongora [:gioyik]]

Not setting review? to Jared due seems he is PTO.

If this looks good, I could push it to try server, I only need the try syntax.

Thanks

Comment 9

[:Gijs (he/him)]

Comment on attachment 8645537 [details] [diff] [review]
1186346.patch

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

Nice! I don't think this needs a try-push, will land it for you in a second.

Comment 10

[:Gijs (he/him)]

I changed the commit message slightly.

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6bc329425006