Note: There are a few cases of duplicates in user autocompletion which are being worked on.

browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Test timed out

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: miker)

Tracking

({intermittent-failure})

Trunk
Firefox 9
x86
Windows XP
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][styleinspector][has-patch][review+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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 hidden (Treeherder Robot)

Updated

6 years ago
Assignee: nobody → getify
Comment hidden (Treeherder Robot)
I have seen this issue before. It seems that sometime $('nodeId') returns undefined in the web console.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
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
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).

Updated

6 years ago
Assignee: getify → mratcliffe

Comment 9

6 years ago
Looks like Mike and Mihai have beaten me to the punch on this one. reassigning to mike. :)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Created attachment 557957 [details] [diff] [review]
Mochitest timeout patch 1

Patch green on try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3ba2b5869de8
Attachment #557957 - Flags: review?(mihai.sucan)
Status: NEW → ASSIGNED
Depends on: 683320
Whiteboard: [orange] → [orange][minotaur][styleinspector][has-patch]
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.
Attachment #557957 - Flags: review?(mihai.sucan) → review-
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
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
Attachment #557957 - Attachment is obsolete: true
Attachment #558174 - Flags: review?(mihai.sucan)
Blocks: 683672
(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 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.
Attachment #558174 - Flags: review?(mihai.sucan) → review-
Created attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

Done
Attachment #558174 - Attachment is obsolete: true
Attachment #558275 - Flags: review?(mihai.sucan)
Comment on attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

Thank you! R+!

Hopefully this fixes the timeouts on winxp.
Attachment #558275 - Flags: review?(mihai.sucan) → review+
Comment hidden (Treeherder Robot)
Blocks: 438871
Status: ASSIGNED → NEW
Whiteboard: [orange][minotaur][styleinspector][has-patch] → [orange][minotaur][styleinspector][has-patch][review+][land-on-fx-team]
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(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/
Whiteboard: [orange][minotaur][styleinspector][has-patch][review+][land-on-fx-team] → [orange][minotaur][styleinspector][has-patch][review+][fixed-in-fx-team]
Comment on attachment 558275 [details] [diff] [review]
[in-fx-team] Mochitest timeout patch 3

http://hg.mozilla.org/integration/fx-team/rev/ab1e3be27b43
Attachment #558275 - Attachment description: Mochitest timeout patch 3 → [in-fx-team] Mochitest timeout patch 3
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
http://hg.mozilla.org/mozilla-central/rev/ab1e3be27b43
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [orange][minotaur][styleinspector][has-patch][review+][fixed-in-fx-team] → [orange][minotaur][styleinspector][has-patch][review+]
Target Milestone: --- → Firefox 9
Keywords: intermittent-failure
Whiteboard: [orange][minotaur][styleinspector][has-patch][review+] → [minotaur][styleinspector][has-patch][review+]
You need to log in before you can comment on or make changes to this bug.