Closed Bug 683737 Opened 13 years ago Closed 13 years ago

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

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 9

People

(Reporter: mayhemer, Assigned: miker)

References

Details

(Keywords: intermittent-failure, Whiteboard: [minotaur][styleinspector][has-patch][review+])

Attachments

(1 file, 2 obsolete files)

[in-fx-team] Mochitest timeout patch 3
2.72 KB, patch
msucan
: review+
Details | Diff | Splinter Review 
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
Assignee: nobody → getify
I have seen this issue before. It seems that sometime $('nodeId') returns undefined in the web console.
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).
Assignee: getify → mratcliffe
Looks like Mike and Mihai have beaten me to the punch on this one. reassigning to mike. :)
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-
Attached patch Mochitest timeout patch 2 (obsolete) — — Splinter Review 
(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)
(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-
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+
Blocks: 438871
Status: ASSIGNED → NEW
Whiteboard: [orange][minotaur][styleinspector][has-patch] → [orange][minotaur][styleinspector][has-patch][review+][land-on-fx-team]
(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
http://hg.mozilla.org/mozilla-central/rev/ab1e3be27b43
Status: NEW → RESOLVED
Closed: 13 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
Whiteboard: [orange][minotaur][styleinspector][has-patch][review+] → [minotaur][styleinspector][has-patch][review+]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: