Closed Bug 1222991 Opened 5 years ago Closed 3 years ago

Style editor doesn't load stylesheets on http://photobucket.com

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox45 affected)

RESOLVED DUPLICATE of bug 1224558
Tracking Status
firefox45 --- affected

People

(Reporter: arni2033, Assigned: pbro)

References

()

Details

(Whiteboard: [dupeme])

User Story

To reproduce: open one of these sites, open Style Editor, reload the page.
> 1) http://www.valuewalk.com/
> 2) http://nypost.com/
> 3) http://photobucket.com/

Result: No stylesheets are displayed

Workaround (only for Photobucket): close and reopen devtools after page load.

Attachments

(3 files)

STR:   (Win7_64, Nightly 45, 32bit, ID 20151108030417, new profile)
1. Open http://photobucket.com/ , wait until it's fully loaded
2. Open devtools -> Style editor (Shift+F7)

Result:
 Style editor keeps loading infinitely; No stylesheets are available; I can't create new stylesheet

Expectations: 
 It should work OK: to load all stylesheets and display them.
2 things that could help investigation:

- this works fine in an non-e10s window,
- in an e10s window, if you close the tab after a while, the following error is displayed in the console:

Error: Connection closed, pending request to server1.conn19.child1/styleSheetsActor8, type getStyleSheets failed

Request stack:
Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
StyleEditorUI.prototype.initialize<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:125:29
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
TaskImpl_run@resource://gre/modules/Task.jsm:319:13
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.getHighlighterUtils/exported.getHighlighterByType<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox-highlighter-utils.js:286:27
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Task_spawn@resource://gre/modules/Task.jsm:164:12
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:381:1
TaskImpl_run@resource://gre/modules/Task.jsm:322:13
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
TaskImpl_run@resource://gre/modules/Task.jsm:319:13
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
Toolbox.prototype.initInspector/this._initInspector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1802:37
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.InspectorFront<.getWalker<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3780:12
Toolbox.prototype.initInspector/this._initInspector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1792:30
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Task_spawn@resource://gre/modules/Task.jsm:164:12
Toolbox.prototype.initInspector@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1790:29
StyleEditorUI.prototype.initializeHighlighter<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:134:11
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
StyleEditorUI.prototype.initialize<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:121:11
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
StyleEditorPanel.prototype.open<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleeditor/styleeditor-panel.js:69:11
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:451:5
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:388:7
TaskImpl_run@resource://gre/modules/Task.jsm:322:13
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1236:19

Stack trace :
Front<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1125:23
StyleEditorPanel.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleeditor/styleeditor-panel.js:140:7
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
TabTarget.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:563:5
TabTarget.prototype.handleEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:524:9
_beginRemoveTab@chrome://browser/content/tabbrowser.xml:2234:13
removeTab@chrome://browser/content/tabbrowser.xml:2117:18
onxblclick@chrome://browser/content/tabbrowser.xml:5849:1
Wait, you gave me an idea to test it a little better. If you have some time, please confirm the following and tell if I should create new bug (about uBlock) and modify this (and how)

1) It always happen, in e10s mode and in non-e10s mode, IF I open style editor BEFORE the page loads
So, STR_1:
 1. Open http://photobucket.com/
 2. Open Style Editor (Shift+F7)
 3. Reload the page, wait until it's fully loaded (DON'T cancel loading by clicking [x] in urlbar)
Result_1:
 Stylesheets aren't displayed; Style editor is loading infinitely.
Workaround_1:
 If I close and reopen devtools, then all stylesheets load fine.

2) It was reproducible for me on Dev.Edition 44 in non-e10s mode with_uBlock_enabled uBlock didn't
   block any CSS, it just blocked some ads. When I disabled uBlock on that page, everything was OK
So, STR_2:
 1. Install uBlock Origin ( https://addons.mozilla.org/ru/firefox/addon/ublock-origin/ )
 2. Open http://photobucket.com/ , wait until it's fully_loaded
 3. Open devtools -> Style editor (Shift+F7)
Result_2:
 Stylesheets aren't displayed; Style editor is loading infinitely.

In both cases, when I close devtools, it throws errors like in comment 1.
(In reply to arni2033 from comment #2)
> Wait, you gave me an idea to test it a little better. If you have some time,
> please confirm the following and tell if I should create new bug (about
> uBlock) and modify this (and how)
> 
> 1) It always happen, in e10s mode and in non-e10s mode, IF I open style
> editor BEFORE the page loads

I think this part is bug 947119.
I investigated this issue a little and found out that:
- StyleSheetsActor.getStyleSheets gets called correctly when the style-editor opens for the first time,
- this method loops over all windows in the page (all iframes) recursively and adds stylesheets from them all,
- when it finds a window, it calls _addStyleSheets(win) which, in turn, waits for the window to finish loading in order to retrieve all the stylesheets from it,
- it turns out that on this site, one of the iframe has no [src] attribute, and so it defaults to about:blank, which never emits the DOMContentLoaded event.

So the getStyleSheets method ends up being blocked waiting for this iframe to load forever.
Looks like this could be special-cased quite easily.
Some related work was done in bug 1171919.
See Also: → 1171919
Sami, you worked on bug 1171919, maybe you have an idea. I can't figure this out.
In some cases (all the times on http://photobucket.com/, but maybe also on other sites, in less frequent cases), an iframe is in uninitialized readyState, but its DOMContentLoaded event never fires.
This happens here with an iframe that has no src (so, about:blank), but I'm unable to create a reduced test case. Whenever I do, even the about:blank iframes end up firing a DOMContentLoaded event.
Flags: needinfo?(sjakthol)
In the meantime, here's a simple patch to avoid going over about:blank iframes.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8687225 - Flags: review?(bgrinstead)
Comment on attachment 8687225 [details] [diff] [review]
Bug_1222991_-_Dont_wait_for_about_blank_windows_to.diff

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

::: devtools/server/actors/stylesheets.js
@@ +104,5 @@
>      let actors = [];
>  
>      for (let win of windows) {
> +      // No need to try and get stylesheets for about:blank windows.
> +      if (win.location.href === "about:blank") {

I wonder if this should be special cased as an early return in _addStyleSheets (closer to the actual source of the bug).  The only difference would be it would then querySelectorAll for children frames, but I can't imagine that would ever return anything besides an empty array for an about:blank window.  What do you think?

@@ +159,5 @@
>     *
>     * @return {Promise}
>     *         Promise that resolves to an array of StyleSheetActors
>     */
> +  _addStyleSheets: Task.async(function*(win) {

Hard to tell in splinter, but this function is unchanged except for switching to Task.async from Task.spawn, right?
Attachment #8687225 - Flags: review?(bgrinstead) → review+
Let's wait to hear back from Sami though, because this could reintroduce the intermittent fixed by Bug 1171919, as per: https://bugzilla.mozilla.org/show_bug.cgi?id=1171919#c6
I don't know what's going on with the original report but I might have found something.

If an iframe fails to load due to a some kind of internal blocker (e.g. an ad blocker, mixed content blocker or maybe the built-in tracking protection), the iframe will have an about:blank content document with readyState === uninitialized that will never load.

For example, I run the following in the web console of a Bugzilla tab
> var frame = document.createElement("iframe")
> frame.src = "http://photobucket.com/"
> document.body.appendChild(frame)
the mixed content blocker will block the iframe from loading and frame.contentDocument.location == "about:blank" and frame.contentDocument.readyState == "uninitialized" forever.

However, the original reporter don't seem to have any ad blockers enabled and photobucket.com is not a https protected where mixed content blocker would act, the issue there might be different.

Anyway, I have no idea how to fix this issue. It's likely to require platform changes (bug 1191683) that would make the readyState values conform to relevant standards (namely, uninitialized is not a valid readyState, http://www.w3.org/TR/html5/dom.html#current-document-readiness). Fixing bug 1224558 would be a good start as these kind of issues would not block the whole tool from loading...

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Let's wait to hear back from Sami though, because this could reintroduce the
> intermittent fixed by Bug 1171919, as per:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1171919#c6
Yes, skipping about:blank documents will most likely reintroduce that intermittent.
Flags: needinfo?(sjakthol)
I wonder if making the change in Bug 1224558 would fix this, specifically using the tabActor list of windows instead of traversing frames.  We may also need to change initialization procedure as in https://bugzilla.mozilla.org/show_bug.cgi?id=1224558#c1, so that for cases where the DOMContentLoaded never fires we don't ever send an event with the sheet and just make sure to clean up any pending handlers on destroy.  That's probably the 'right' way to fix this issue.
Style Editor stays empty on those 2 sites even if I open it AFTER page load. Just listing all 3 sites.
User Story: (updated)
I agree that the right devtools fix here would be bug 1224558, but I think we should go to the bottom of why some content windows in iframes on the test site do not emit DOMContentLoaded events.

Olli, your help would be great. The site is : http://photobucket.com/
Sometimes during the load of this website, devtools goes through the DOM to find all iframes, and when it does find one, it checks its readyState, if it's "uninitialized", then it waits for the its contentWindow's DOMContentLoaded event.
We've found that this event does not always happen, and would like to understand why.

My, really ugly, way of testing this simply is:
- open this website,
- open the browser console (Ctrl+Shift+J),
- and then paste the code below into the scratchpad (Shift+F4, but first changing the environment preset of scratchpad to "browser")

--------------------------------------
function getWin() {
  return gBrowser.selectedTab.linkedBrowser._contentWindow;
}
getWin().location.reload();

// wait for some time before going through the DOM, 4s happen to work for me.
setTimeout(() => {
  let win = getWin();
  let doc = win.document;
  console.log(">> Found ", doc.querySelectorAll("iframe").length, " iframes");

  for (let frame of doc.querySelectorAll("iframe, frame")) {
    let content = frame.contentWindow;
    if (content.document.readyState === "uninitialized") {
      console.log(">> Found iframe src=", content.location.href, " with uninitialized state, listening for DOMContentLoaded");
      content.addEventListener("DOMContentLoaded", function onLoad() {
        content.removeEventListener("DOMContentLoaded", onLoad);
        console.log(">> OK!!");
      });
    }
  }
}, 4000);
--------------------------------------

In the browser console, I can see that 9 iframes are detected, and that 2 of them are about:blank and uninitialized readyState, but their DOMContentLoaded listeners never gets called.
Flags: needinfo?(bugs)
I guess you have about:blank iframes here?

If so, sounds like we should initialize about:blanks, at least those one docshell create, to 
"complete". Bz, what would you think of that?
Hmm, need to check what the spec says about this.
Flags: needinfo?(bugs) → needinfo?(bzbarsky)
I know Henri tried to do some stuff around that and it caused failures up the wazoo, but I'm not sure what stuff he tried....

In any case, seems like "interactive" would be the right value, not "complete", since we do still plan to fire onload and presumably transition to "complete" at that point, right?
Flags: needinfo?(bzbarsky)
Oh, and needinfo on Henri per comment 16.
Flags: needinfo?(hsivonen)
(not sure whether "interactive" or "complete" would be right).

Anyhow, if changing readyState handling is too scary, perhaps we could add yet another ChromeOnly
property to Document. Something similar to isSrcdocDocument. isInitialAboutBlank ?
(assuming this bug is about initial about blank).
"When a Document object is created, it must have its current document readiness set to the string "loading" if the document is associated with an HTML parser, an XML parser, or an XSLT processor, and to the string "complete" otherwise."
(In reply to Olli Pettay [:smaug] from comment #18)
> (assuming this bug is about initial about blank).
Not sure what is meant by "initial about blank", but on the test page I mentioned in comment 14, and following the STRs in the same comment, I see 2 about:blank windows being logged, but I know for a fact that on that site, one of the iframes has no src attribute.
Executing this in the web console
[...document.querySelectorAll("iframe")].map(i => i.getAttribute("src"))
prints an array with one item being null
I suppose that is't reproducible on faster/slower PCs, if you change the delay in setTimeout
We are nowhere near spec-compliant on about:blank. Additionally, I worry that adding more hacks to our current bogus solution will make it even more bogus.

To make progress on non-bogus about:blank, we need to find someone who has the time to implement bug 779959 comment 22. That's needed to make the readyState assertions right. Fixing about:blank itself is pretty hopeless if one can't rely on those assertions being right.
Flags: needinfo?(hsivonen)
See Also: → 1227703
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
This issue has been reported on uBlock Origin's issue tracker[1]. I narrowed the issue, and it occurs when an iframe on a page is blocked by uBlock Origin. I tried with ABP and the issue is also occurring.

I created a small test case to reproduce, here is the HTML code:

    <!DOCTYPE html>
    <html>
    <head></head>
    <body>
    <iframe src="https://www.example.com/ad-frame.html"></iframe>
    </body>
    </html>

The HTML content above can be loaded in the browser with the test case hosted on my personal server:
http://www.raymondhill.net/ublock/bugzilla1222991/

Steps to reproduce:

1. Start Nightly or Firefox 50.
2. Install uBlock Origin, or Adblock Plus (default settings for both will do).
3. Load the web page http://www.raymondhill.net/ublock/bugzilla1222991/
4. Observation: the page should be blank, as the embedded iframe is blocked by an EasyList filter.
5. Open the dev tools for the page (Ctrl-Shift-I).
6. Select the "Style Editor" pane.
7. Observation: a sticky spinning icon which never goes away.
8. Open the browser console (Ctrl-Shift-J).
9. Close the dev tools for the page.
10. Observation: notice the errors reported in the browser console -- similar to what is reported in comment #1.
11. Disable the blocker for the site.
12. Force-reload the page.
13. Open the dev tools for the page (Ctrl-Shift-I).
14. Select the "Style Editor" pane.
15. Observation: the styles are loaded, no sticky spinning icon.
16. Close the dev tools for the page.
17 Observation: no error dumped at the browser console.

So apparently the blocking of an iframe on a page is what causes the issue.

***

[1] https://github.com/gorhill/uBlock/issues/2169
See Also: → 1327344
This will be fixed by bug 1224558.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1224558
No longer blocks: top-inspector-bugs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.