Closed Bug 1171919 Opened 7 years ago Closed 7 years ago

Intermittent browser_styleeditor_bug_740541_iframes.js | Test timed out - expected PASS


(DevTools :: Style Editor, defect)

Not set


(firefox40 unaffected, firefox41 fixed, firefox42 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected


(Reporter: cbook, Assigned: sjakthol)




(Keywords: intermittent-failure)


(2 files, 2 obsolete files)


21:26:19 INFO - 4717 INFO TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_bug_740541_iframes.js | Test timed out - expected PASS
Debugged this a bit during the weekend and discovered that occasionally some of the iframes haven't began to load when DOMContentLoaded arrives (i.e. their location is about:blank and readyState complete) so their sheets will be missing from the results.

This explains the failure in comment 4 but the timeouts are still a mystery.
I think I figured this out.

When an iframe is created, it is given a dummy about:blank document with readyState === "uninitialized" at first. When the frame starts to load that dummy document is replaced with a new one. If we happen find the frame between at just the right moment we store the dummy document instead of the real one. When we go to access the document later, we either a) find no sheets or b) try to access a dead object and both lead to a failure.

The fix is to track the windows of the frames instead of the documents as windows don't seem to change during load.
Attached patch bug-1171919-iframe-loading.patch (obsolete) — Splinter Review
Here's a patch that could fix the failures if my thoughts in the previous comment are correct.

Assignee: nobody → sjakthol
Attachment #8632529 - Flags: review?(bgrinstead)
Comment on attachment 8632529 [details] [diff] [review]

Review of attachment 8632529 [details] [diff] [review]:

Switching to window instead of document seems fine.  It seems like this is super intermittent so without an easily reproducible test case it's going to be hard to say for sure if this fixes it, but it seems pretty low-risk thing to try.  Please see the one comment below

::: toolkit/devtools/server/actors/stylesheets.js
@@ +87,5 @@
>        actors = actors.concat(sheets);
>        // Recursively handle style sheets of the documents in iframes.
> +      for (let iframe of win.document.querySelectorAll("iframe, browser, frame")) {
> +        if (iframe.contentWindow) {

Should this condition change?  From the comment below:

// Sometimes, iframes don't have any document, like the	
// one that are over deeply nested (bug 285395)

In that case, does the frame also not have a contentWindow?  We should either add a test case that covers this 'deeply nested' case so we know for sure, or keep it checking iframe.contentDocument just to be safe.
Attachment #8632529 - Flags: review?(bgrinstead)
Ah, yes. I think it's best to ensure the frame has both contentWindow and contentDocument defined as we are using them both now. Here's an updated version of the patch.

I'll also attach a second patch that contains a test for the nested iframe handling.
Attachment #8632529 - Attachment is obsolete: true
Attachment #8633606 - Flags: review?(bgrinstead)
Here's the test for nested frames.

Pending try run:
Attachment #8633610 - Flags: review?(bgrinstead)
Comment on attachment 8633610 [details] [diff] [review]

Review of attachment 8633610 [details] [diff] [review]:

Thanks for adding the test, r=me with the comment addressed

::: toolkit/devtools/server/tests/browser/stylesheets-nested-iframes.html
@@ +16,5 @@
> +    let iframe = document.querySelector("iframe");
> +    let i = location.href.split("?")[1] || 0;
> +
> +    // The frame can't have the same src URL as any of its ancestors.
> +    iframe.src = location.href + "?" + (i++);

This ends up constructing URLs like

Which works, but I think you want this (plus added a comment about why it's not going to cause infinite recursion)

    let iframe = document.querySelector("iframe");
    let i = parseInt(location.href.split("?")[1]) || 1;

    // The frame can't have the same src URL as any of its ancestors.
    // This will not infinitely recurse because a frame won't get a content document
    // once it's nested deeply enough.
    iframe.src = location.href.split("?")[0] + "?" + (i+1);
Attachment #8633610 - Flags: review?(bgrinstead) → review+
Comment on attachment 8633606 [details] [diff] [review]

Review of attachment 8633606 [details] [diff] [review]:

Looks good to me
Attachment #8633606 - Flags: review?(bgrinstead) → review+
Heh, I realized the problem later and didn't have time to fix it at that moment. Here's a revised version of the patch based on your suggestions. Thanks for the quick reviews!
Attachment #8633610 - Attachment is obsolete: true
Attachment #8634152 - Flags: review+
Keywords: checkin-needed
Please request Aurora approval on this when you get a chance :)
Comment on attachment 8633606 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Minimal. In very, very rare cases Style Editor might not work for pages with iframes that happen to be parsed at just the right moment. Uplift would mainly impact sheriffs' work as the patch fixes an intermittent failure.
[Describe test coverage new/current, TreeHerder]: Automated tests are still passing. Manually tested that Style Editor still works (loads and shows sheets).
[Risks and why]: The code touches the code that lists style sheets for the Style Editor. This could cause Style Editor to not show some style sheets or no sheets at all in the worst case.
[String/UUID change made/needed]: None.
Flags: needinfo?(sjakthol)
Attachment #8633606 - Flags: approval-mozilla-aurora?
This request has been pending for nearly a week. Can we please get a decision here?
Flags: needinfo?(rkothari)
Comment on attachment 8633606 [details] [diff] [review]

Automated test pass and this has been in m-c for a while, let's land it on Aurora.
Flags: needinfo?(rkothari)
Attachment #8633606 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1222991
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.