Closed Bug 1171919 Opened 9 years ago Closed 9 years ago

Intermittent browser_styleeditor_bug_740541_iframes.js | Test timed out - expected PASS

Categories

(DevTools :: Style Editor, defect)

Product:
DevTools
Component:
Style Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: cbook, Assigned: sjakthol)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

bug-1171919-iframe-loading.patch
4.39 KB, patch
bgrins
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
bug-1171919-nested-iframes-test.patch
4.43 KB, patch
sjakthol
: review+
Details | Diff | Splinter Review 
mozilla-inbound_snowleopard-debug_test-mochitest-devtools-chrome-4

https://treeherder.mozilla.org/logviewer.html#?job_id=10461265&repo=mozilla-inbound

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.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f689087ef50
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8632529 - Flags: review?(bgrinstead)
Comment on attachment 8632529 [details] [diff] [review]
bug-1171919-iframe-loading.patch

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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=113e753b4a66
Attachment #8633610 - Flags: review?(bgrinstead)
Comment on attachment 8633610 [details] [diff] [review]
bug-1171919-nested-iframes-test.patch

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 http://test1.example.org/browser/toolkit/devtools/server/tests/browser/stylesheets-nested-iframes.html?0?0?0?0.

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]
bug-1171919-iframe-loading.patch

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 :)
status-firefox40: --- → unaffected
status-firefox41: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Flags: needinfo?(sjakthol)
Comment on attachment 8633606 [details] [diff] [review]
bug-1171919-iframe-loading.patch

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]
bug-1171919-iframe-loading.patch

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.

Attachment

General

Creator:
Created:
Updated:
Size: