913641 - "TypeError: this.markup is undefined: InspectorPanel_onNavigatedAway/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:312" on all mochitest-bc runs

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

TBPL says this is a regression from bug 879578.

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

Ubuntu VM 12.04 fx-team opt test mochitest-browser-chrome on 2013-09-06 04:01:22 PDT for push bf671d77bdad
slave: tst-linux32-ec2-344

04:26:56     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_select_last_selected.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://mochi.test:8888/browser/browser/devtools/inspector/test/browser_inspector_select_last_selected2.html" line: 0}]
04:26:56     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_select_last_selected.js | Node re-selected after navigation
04:26:56     INFO -  JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js, line 1044: this.elementStyle is undefined
04:26:56     INFO -  TypeError: this.markup is undefined: InspectorPanel_onNavigatedAway/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:312
04:26:56     INFO -  @resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:63
04:26:56     INFO -  EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:107
04:26:56     INFO -  InspectorPanel__onMarkupFrameLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:606
04:26:56     INFO -  InspectorPanel_initMarkupPanel_onload@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:588
04:26:56     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_select_last_selected.js | Console message: [JavaScript Error: "TypeError: this.elementStyle is undefined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js" line: 1044}]
04:26:57     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_select_last_selected.js | finished in 4805ms

Comment 1

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=27513736&tree=Mozilla-Inbound

Comment 2

[Patrick Brosset <:pbro>]

This only seem to happen in this test I added for bug 879578.
Indeed, when similar a scenario is executed by a user, steps are not executed in the same event loop as when the markup-view is created or destroyed.

One of the errors I've identified comes from calling MarkupView._updateChildren after MarkupView has been destroyed.
Indeed, in its `detroy` function, MV does `this._containers`, and in `_updateChildren` MV does `if(!this._containers) {return promise.reject("markup view destroyed");}`.

I've managed to fix the failing test by adding many executeSoon(...). But it feels wrong to have to do this, however I can't figure out a better way at this stage (other than using setTimeout(..., 1000)).

Attaching the patch now to ask for help.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: 913641-reslect-node-on-reload-test-failure.patch

Paul, could you please help me out here?

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 801419 [details] [diff] [review]
913641-reslect-node-on-reload-test-failure.patch

Transferring review to Mike as Paul won't have time now.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 801419 [details] [diff] [review]
913641-reslect-node-on-reload-test-failure.patch

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

::: browser/devtools/inspector/test/browser_inspector_select_last_selected.js
@@ +101,5 @@
>    function testSameNodeSelectedOnNavigateAwayAndBack() {
>      // On page2, select id5
>      let id = "id5";
>      let div = content.document.getElementById(id);
> +    

Nit: Whitespace

Comment 6

[Patrick Brosset <:pbro>]

Thanks Mike for the review.
Attaching a new patch now to remove the whitepsace.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: 913641-reslect-node-on-reload-test-failure.patch

Whitespace removed.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/169a0df8cdb4

FWIW, [land-in-fx-team] is needless these days IMO given that we're using fx-team as another integration branch that's merged 1-2 times daily to m-c.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/169a0df8cdb4