966572 - "TypeError: this._itemIds is undefined -- chrome://browser/content/browser.js:3394" on all mochitest-bc runs

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Started with the push for bug 964887.

https://tbpl.mozilla.org/php/getParsedLog.php?id=33911990&tree=Fx-Team

16:10:02     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2.js
16:10:02     INFO -  console.error:
16:10:02     INFO -    [CustomizableUI]
16:10:02     INFO -    TypeError: this._itemIds is undefined -- chrome://browser/content/browser.js:3394
16:10:02     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2.js | Console message: [JavaScript Error: "this.star is null" {file: "chrome://browser/content/browser.js" line: 3075}]
16:10:02     INFO -  TEST-INFO | unknown test url | must wait for load
16:10:02     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2.js | sanity check
16:10:02     INFO -  TEST-PASS | unknown test url | popuphidden 0
16:10:03     INFO -  TEST-PASS | unknown test url | popuphidden 1
16:10:03     INFO -  TEST-PASS | unknown test url | popuphidden 2
16:10:03     INFO -  TEST-PASS | unknown test url | popuphidden 3
16:10:04     INFO -  TEST-PASS | unknown test url | popuphidden 3
16:10:04     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2.js | Console message: [JavaScript Error: "this.star is null" {file: "chrome://browser/content/browser.js" line: 3070}]
16:10:04     INFO -  TEST-INFO | MEMORY STAT vsize after test: 703344640
16:10:04     INFO -  TEST-INFO | MEMORY STAT residentFast after test: 231964672
16:10:04     INFO -  TEST-INFO | MEMORY STAT heapAllocated after test: 106790368
16:10:04     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2.js | finished in 2144ms

And a few more like it.

Comment 1

[:Gijs (he/him)]

Ugh. Sorry, I was asleep. Looking.

Comment 2

[:Gijs (he/him)]

NB: note that this exception might not fall in the category of bug 920191 because it is caught and then console.error()'d by CustomizableUI. Basically, AFAICT it's fired from a listener registered with CustomizableUI, which catches exceptions thrown by listeners and dumps them into console.error so as not to disrupt its own operations. I wouldn't have expected window.onerror to catch these.

Comment 3

[:Gijs (he/him)]

Attachment: always initialize itemIds and continue nullchecking this.star,

Early on startup this.star is still null because the binding hasn't been initialized, especially if the star node gets overflown into the overflow panel. Similarly, this._itemIds is still undefined. This patch fixes both of these issues. Marco, does this look OK to you?

Comment 4

[:Gijs (he/him)]

(note that the logs also have this.star is null errors, they're just not reported in the log as being broken, don't know why)

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8369047 [details] [diff] [review]
always initialize itemIds and continue nullchecking this.star,

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

::: browser/base/content/browser-places.js
@@ +1086,5 @@
>    /**
>     * Handles star styling based on page proxy state changes.
>     */
>    onPageProxyStateChanged: function BUI_onPageProxyStateChanged(aState) {
> +    if (!this._shouldUpdateStarState() || !this.star) {

The _itemIds change looks very sane, though for this.starm I'd suggest making _shouldUpdateStarState return false if !this.star

Comment 6

[:Gijs (he/him)]

Landed with minor changes as discussed on IRC:

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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