Last Comment Bug 683737 - browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
: browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsol...
Status: RESOLVED FIXED
[minotaur][styleinspector][has-patch]...
: intermittent-failure
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 683320
Blocks: 438871 683672
  Show dependency treegraph
 
Reported: 2011-08-31 14:43 PDT by Honza Bambas (:mayhemer)
Modified: 2012-11-25 19:31 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mochitest timeout patch 1 (3.22 KB, patch)
2011-09-02 15:11 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review-
Details | Diff | Splinter Review
Mochitest timeout patch 2 (3.11 KB, patch)
2011-09-04 11:32 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review-
Details | Diff | Splinter Review
[in-fx-team] Mochitest timeout patch 3 (2.72 KB, patch)
2011-09-05 06:34 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-08-31 14:43:35 PDT
Just failed on Win XP opt:

http://hg.mozilla.org/integration/mozilla-inbound/rev/891e5dbae3ec
https://tbpl.mozilla.org/?usebuildbot=1&tree=Mozilla-Inbound&rev=891e5dbae3ec
https://tbpl.mozilla.org/php/getParsedLog.php?id=6217020

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_netlogging.js | Test timed out
Comment 1 Treeherder Robot 2011-08-31 15:35:05 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6218137
Rev3 WINNT 5.1 mozilla-central opt test mochitest-other on 2011-08-31 14:33:02

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 2 Treeherder Robot 2011-09-01 08:15:27 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6229882
Rev3 WINNT 5.1 mozilla-inbound opt test mochitest-other on 2011-09-01 07:31:47

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-01 09:58:23 PDT
I have seen this issue before. It seems that sometime $('nodeId') returns undefined in the web console.
Comment 4 Treeherder Robot 2011-09-01 17:26:49 PDT
dholbert
https://tbpl.mozilla.org/php/getParsedLog.php?id=6237900
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-other on 2011-09-01 16:09:07

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 5 Treeherder Robot 2011-09-01 17:26:55 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6237900
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-other on 2011-09-01 16:09:07

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 6 Treeherder Robot 2011-09-01 21:48:07 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6238627
Rev3 WINNT 5.1 mozilla-inbound opt test mochitest-other on 2011-09-01 17:49:41

9463 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/editor/composer/test/test_bug678842.html | unexpected lang en-US - got en-US, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-02 06:08:29 PDT
Here is another possibility of what triggers the error:

1. An iframe is created in the style panel that is then opened

2. A new instance of CssHtmlTree is created

3. The CssHtmlTree constructor does the following:
  - this.styleDocument = iframe.contentWindow.document;
  - this.root = this.styleDocument.getElementById("root");

3. SI_popupShown calls this.cssHtmlTree.highlight();

4. CssHtmlTree.highlight calls CssHtmlTree.processTemplate(this.templateRoot, this.root, this);

5. CssHtmlTree.processTemplate attempts to set this.root.innerHTML = "" but fails because this.root is undefined
Comment 8 Mihai Sucan [:msucan] 2011-09-02 06:19:12 PDT
I believe that analysis is correct, Mike.

The fix is easy: in StyleInspector.jsm add an event listener for the iframe load event. It seems that popupshown is triggered before the iframe gets the chance to load, and CssHtmlTree breaks (as it would be expected when .root is null).
Comment 9 Kyle Simpson 2011-09-02 06:30:35 PDT
Looks like Mike and Mihai have beaten me to the punch on this one. reassigning to mike. :)
Comment 10 Treeherder Robot 2011-09-02 06:38:01 PDT
mfinkle%mozilla.com
https://tbpl.mozilla.org/php/getParsedLog.php?id=6243846
Rev3 WINNT 5.1 mozilla-central debug test mochitest-other on 2011-09-02 04:37:10

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 11 Treeherder Robot 2011-09-02 14:35:16 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6252750
Rev3 WINNT 5.1 mozilla-central debug test mochitest-other on 2011-09-02 12:58:22

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-02 15:11:13 PDT
Created attachment 557957 [details] [diff] [review]
Mochitest timeout patch 1

Patch green on try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3ba2b5869de8
Comment 13 Mihai Sucan [:msucan] 2011-09-03 03:23:16 PDT
Comment on attachment 557957 [details] [diff] [review]
Mochitest timeout patch 1

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

General comments:

- This patch does not apply cleanly on top of bug 683320, on fx-team. Please rebase.

- With this patch SI_popupShown() can execute twice: if the iframe load event fires followed by popupshown. Please make sure this can't happen.

More comments below. Looking forward for the updated patch. Thank you!

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +110,5 @@
> +     */
> +    function SI_iframeOnload() {
> +      panel.cssLogic = new CssLogic();
> +      panel.cssHtmlTree = new CssHtmlTree(iframe, panel.cssLogic, panel);
> +      panel.iframeReady = true;

I believe there's no need to add iframeReady to panel. Please use something like:

let iframeReady = false;
function SI_iframeOnload() {
  // ...
  iframeReady = true;
  // ...
}

Then you can use iframeReady in SI_popupShown() as well.

@@ +111,5 @@
> +    function SI_iframeOnload() {
> +      panel.cssLogic = new CssLogic();
> +      panel.cssHtmlTree = new CssHtmlTree(iframe, panel.cssLogic, panel);
> +      panel.iframeReady = true;
> +      SI_popupShown.apply(panel);

Please use .call() here, instead of .apply().

@@ +175,5 @@
>        this.cssLogic = null;
>        this.cssHtmlTree = null;
>        this.removeEventListener("popupshown", SI_popupShown);
>        this.removeEventListener("popuphidden", SI_popupHidden);
> +      iframe.removeEventListener("load", SI_iframeOnload, true);

Please move this line into the SI_iframeOnload() function.
Comment 14 Treeherder Robot 2011-09-03 06:56:54 PDT
khuey
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1315049104.1315053445.11543.gz
Rev3 WINNT 5.1 mozilla-central debug test mochitest-other on 2011/09/03 04:25:04

s: talos-r3-xp-033
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 15 Treeherder Robot 2011-09-03 12:23:14 PDT
Callek
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1315069144.1315070608.30731.gz
Rev3 WINNT 5.1 mozilla-central opt test mochitest-other on 2011/09/03 09:59:04

s: talos-r3-xp-018
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 16 Treeherder Robot 2011-09-03 12:23:37 PDT
Callek
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1315064156.1315068508.26194.gz
Rev3 WINNT 5.1 mozilla-central debug test mochitest-other on 2011/09/03 08:35:56

s: talos-r3-xp-040
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-04 11:32:48 PDT
Created attachment 558174 [details] [diff] [review]
Mochitest timeout patch 2

(In reply to Mihai Sucan [:msucan] from comment #13)
> Comment on attachment 557957 [details] [diff] [review]
> Mochitest timeout patch 1
> 
> Review of attachment 557957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - This patch does not apply cleanly on top of bug 683320, on fx-team. Please
> rebase.
> 

Done

> - With this patch SI_popupShown() can execute twice: if the iframe load
> event fires followed by popupshown. Please make sure this can't happen.
> 

Well spotted, I have added another flag to fix this.

> More comments below. Looking forward for the updated patch. Thank you!
> 
> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +110,5 @@
> > +     */
> > +    function SI_iframeOnload() {
> > +      panel.cssLogic = new CssLogic();
> > +      panel.cssHtmlTree = new CssHtmlTree(iframe, panel.cssLogic, panel);
> > +      panel.iframeReady = true;
> 
> I believe there's no need to add iframeReady to panel. Please use something
> like:
> 
> let iframeReady = false;
> function SI_iframeOnload() {
>   // ...
>   iframeReady = true;
>   // ...
> }
> 
> Then you can use iframeReady in SI_popupShown() as well.
> 

I guess these flags should be private, done.

> @@ +111,5 @@
> > +    function SI_iframeOnload() {
> > +      panel.cssLogic = new CssLogic();
> > +      panel.cssHtmlTree = new CssHtmlTree(iframe, panel.cssLogic, panel);
> > +      panel.iframeReady = true;
> > +      SI_popupShown.apply(panel);
> 
> Please use .call() here, instead of .apply().
> 

Done, but what is your reason for this change?

> @@ +175,5 @@
> >        this.cssLogic = null;
> >        this.cssHtmlTree = null;
> >        this.removeEventListener("popupshown", SI_popupShown);
> >        this.removeEventListener("popuphidden", SI_popupHidden);
> > +      iframe.removeEventListener("load", SI_iframeOnload, true);
> 
> Please move this line into the SI_iframeOnload() function.

Done
Comment 18 Mihai Sucan [:msucan] 2011-09-05 04:26:35 PDT
(In reply to Michael Ratcliffe from comment #17)
> Created attachment 558174 [details] [diff] [review]
> Mochitest timeout patch 2
> 
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Comment on attachment 557957 [details] [diff] [review]
> > Mochitest timeout patch 1
> > 
> > Review of attachment 557957 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +111,5 @@
> > > +    function SI_iframeOnload() {
> > > +      panel.cssLogic = new CssLogic();
> > > +      panel.cssHtmlTree = new CssHtmlTree(iframe, panel.cssLogic, panel);
> > > +      panel.iframeReady = true;
> > > +      SI_popupShown.apply(panel);
> > 
> > Please use .call() here, instead of .apply().
> > 
> 
> Done, but what is your reason for this change?

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/call
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/apply

I prefer to see .apply() used when appropriate.

Thanks for the updated patch!
Comment 19 Mihai Sucan [:msucan] 2011-09-05 04:38:00 PDT
Comment on attachment 558174 [details] [diff] [review]
Mochitest timeout patch 2

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

Giving r- for the SI_popupShown() call from the popupshown event handler - that will break. Please see the comment below.

Thanks again for the updated patch!

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +142,5 @@
>      }
>  
> +    panel.addEventListener("popupshown", function() {
> +      panelReady = true;
> +      SI_popupShown();

Hm, this change is not needed. Besides, the call will break: you did not use .call() (the this object will be wrong in SI_popupShown()).

Please put panelReady = true in SI_popupShown(). In SI_iframeOnload() check if (panelReady) before you call SI_popupShown(). In SI_popupShown() update the if() condition to no longer check for panelReady, since that's going to be set to true by the function itself.
Comment 20 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-05 06:34:12 PDT
Created attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

Done
Comment 21 Mihai Sucan [:msucan] 2011-09-05 07:55:10 PDT
Comment on attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

Thank you! R+!

Hopefully this fixes the timeouts on winxp.
Comment 22 Treeherder Robot 2011-09-05 12:18:36 PDT
Ms2ger%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1315233520.1315235353.21758.gz
Rev3 WINNT 6.1 mozilla-central opt test mochitest-other on 2011/09/05 07:38:40

s: talos-r3-w7-045
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 23 Treeherder Robot 2011-09-05 14:10:21 PDT
longsonr
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1315251024.1315255355.15388.gz
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-other on 2011/09/05 12:30:24

s: talos-r3-xp-024
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 24 Treeherder Robot 2011-09-06 02:23:37 PDT
m_kato%ga2.so-net.ne.jp
https://tbpl.mozilla.org/php/getParsedLog.php?id=6287193
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-other on 2011-09-06 00:38:03

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 25 Panos Astithas [:past] 2011-09-06 06:11:19 PDT
(In reply to Mihai Sucan [:msucan] from comment #18)
> (In reply to Michael Ratcliffe from comment #17)
> > (In reply to Mihai Sucan [:msucan] from comment #13)
> > > > +      SI_popupShown.apply(panel);
> > > 
> > > Please use .call() here, instead of .apply().
> > 
> > Done, but what is your reason for this change?
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/
> Function/call
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/
> Function/apply
> 
> I prefer to see .apply() used when appropriate.

Another way to put it is that call() is the more fundamental API, as explained here:

http://yehudakatz.com/2011/08/11/understanding-javascript-function-invocation-and-this/
Comment 26 Rob Campbell [:rc] (:robcee) 2011-09-06 10:53:17 PDT
Comment on attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

http://hg.mozilla.org/integration/fx-team/rev/ab1e3be27b43
Comment 27 Treeherder Robot 2011-09-06 16:58:30 PDT
bill%wg9s.com
https://tbpl.mozilla.org/php/getParsedLog.php?id=6299999
Rev3 WINNT 5.1 mozilla-central opt test mochitest-other on 2011-09-06 16:23:17

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 28 Treeherder Robot 2011-09-06 22:16:50 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6303790
Rev3 WINNT 5.1 mozilla-central opt test mochitest-other on 2011-09-06 20:28:37

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 29 Treeherder Robot 2011-09-07 14:51:47 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=6318098
Rev3 WINNT 6.1 mozilla-inbound debug test mochitest-other on 2011-09-07 13:14:12

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out
Comment 30 Tim Taubert [:ttaubert] 2011-09-08 02:38:06 PDT
http://hg.mozilla.org/mozilla-central/rev/ab1e3be27b43

Note You need to log in before you can comment on or make changes to this bug.