Closed Bug 1170864 Opened 5 years ago Closed 5 years ago

Style Editor hangs when trying to read empty stylesheet

Categories

(DevTools :: Style Editor, defect)

39 Branch
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mozbz, Assigned: sjakthol)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150531030228

Steps to reproduce:

0. Start with a fresh Firefox profile.
1. Navigate to a page that loads an existing, but empty stylesheet.
2. Open the Developer Tools, and switch to the Style Editor.


Actual results:

The Style Editor will populate the stylesheet list until it encounters the empty stylesheet. The spinner will continue, but the empty sheet and any remaining stylesheets are not loaded in to the list.

This error is thrown in the Browser Console:
NS_BASE_STREAM_CLOSED: Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]  - DevToolsUtils.js:484:0


Expected results:

The empty sheet should appear in the list just as an empty <style> element would, and processing should continue.
(In reply to M8R-p7tp8h from comment #0)
> 1. Navigate to a page that loads an existing, but empty stylesheet.

Please provide an example.
Component: Untriaged → Developer Tools: Style Editor
Keywords: testcase-wanted
I was afraid you'd say that. I can't upload empty files to bugzilla and I currently have no hosting, so a zip will have to do.

The zip has three files - an HTML page to @import the styles, an empty (zero-length) style and a second style with content to demonstrate that loading is blocked in the Style Editor.
Thank you for the update.

Last good: 2015-03-28
First bad: 2015-03-29
Push log: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad587ca628cf&tochange=385840329d91
Suspect: bug 1147765
Blocks: 1147765
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 41 Branch → 39 Branch
I can only reproduce this with file:// URLs. Essentially this seem to be caused by bug 982654.
See Also: → 982654
Attached patch bug-1170864-refactor-fetch.patch (obsolete) — Splinter Review
Here's a patch that simplifies the fetch method and fixes this bug in the process.

It does two things:
1) It always does the fetching with NetUtil methods. The two code paths there are pretty much doing the same thing now so I think they can be unified. The earliest version of the method I could find [1] uses cache for some schemes which makes that kind of division necessary. But that was back in 2011 and things have changes since so I think this should work now.
2) It falls back to OS.File if a local file cannot be read by the default protocol handler. This is the actual fix for this bug. OS.File cannot be used by default as it doesn't provide means to get the content type of the file.

The commit message has some more details.

[1] https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/6c672798c534
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8620756 - Flags: feedback?(past)
Attached patch bug-1170864-fetch-tests.patch (obsolete) — Splinter Review
Here's some xpcshell-tests for the fetch method as currently it's not tested directly.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b848fc059dd
Comment on attachment 8620756 [details] [diff] [review]
bug-1170864-refactor-fetch.patch

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

I have a few questions and comments, but overall I'm happy with this.

::: toolkit/devtools/DevToolsUtils.js
@@ +444,5 @@
>   *          always load fresh from the network (default: true)
>   *        - policy: the nsIContentPolicy type to apply when fetching the URL
>   *        - window: the window to get the loadGroup from
>   *        - charset: the charset to use if the channel doesn't provide one
> + * @returns Promise that resolves with an object with following members on

Nit: "with the following"

@@ -451,3 @@
>    let deferred = promise.defer();
> -  let scheme;
> -  let url = aURL.split(" -> ").pop();

Did you remove this because NetUtil.newChannel does it automatically? Have you tested debugging an add-on and see if the funky URLs appear or not?

@@ +488,5 @@
> +      return;
> +    }
> +
> +    try {
> +      let charset = channel.contentCharset || aOptions.charset;

convertToUnicode used to fall back to UTF-8 if no charset was found.

@@ +513,5 @@
> +          let decoder = new TextDecoder();
> +          let content = decoder.decode(bytes);
> +
> +          // We can't detect the contentType without opening a channel
> +          // and that failed already. This is the best we can do here.

Isn't this path going to be taken only if the file is empty? If so, this is not only the best, but arguably the correct content type to use, too. But more importantly, in that case we could have omitted the read(), right?
Attachment #8620756 - Flags: feedback?(past) → feedback+
Thanks for the feedback! I'll update the patch according to your comments.

> @@ -451,3 @@
> >    let deferred = promise.defer();
> > -  let scheme;
> > -  let url = aURL.split(" -> ").pop();
> 
> Did you remove this because NetUtil.newChannel does it automatically? Have
> you tested debugging an add-on and see if the funky URLs appear or not?
I removed this by accident, I'll add it back and write a test for it. I wonder why existing debugger tests didn't catch this one...

> @@ +488,5 @@
> > +      return;
> > +    }
> > +
> > +    try {
> > +      let charset = channel.contentCharset || aOptions.charset;
> 
> convertToUnicode used to fall back to UTF-8 if no charset was found.
nsIConverterInputStream used by NetUtil defaults to it though it might make sense to be explicit if something changes in the future.

https://dxr.mozilla.org/mozilla-central/source/intl/uconv/nsConverterInputStream.cpp#28

> @@ +513,5 @@
> > +          let decoder = new TextDecoder();
> > +          let content = decoder.decode(bytes);
> > +
> > +          // We can't detect the contentType without opening a channel
> > +          // and that failed already. This is the best we can do here.
> 
> Isn't this path going to be taken only if the file is empty? If so, this is
> not only the best, but arguably the correct content type to use, too. But
> more importantly, in that case we could have omitted the read(), right?

NS_BASE_STREAM_CLOSED can be thrown for various other reasons such as the file does not exist or can't be opened due to missing permissions. OS.File.read is required to make a distinction between different errors as it seems to succeed even if the file is empty.

However, I don't know the inner workings of the file:// handler and OS.File implementation so I didn't want to bet on the file being empty as the NS_BASE_STREAM_CLOSED exception could be result of a transient issue which went away before OS.File was called. Making that assumption could lead to bugs like 'debugger shows empty sources' that happen intermittently and are impossible to reproduce. So I though it would be better to try to get the real content by all means necessary instead of making assumptions based on my limited knowledge.
Attached patch bug-1170864-refactor-fetch.patch (obsolete) — Splinter Review
Here's a patch that addresses your feedback.
Attachment #8620756 - Attachment is obsolete: true
Attachment #8621950 - Flags: review?(past)
Here's the updated tests for fetch.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b8536bed0d8
Attachment #8620762 - Attachment is obsolete: true
Attachment #8621953 - Flags: review?(past)
(In reply to Sami Jaktholm from comment #8)
> > @@ -451,3 @@
> > >    let deferred = promise.defer();
> > > -  let scheme;
> > > -  let url = aURL.split(" -> ").pop();
> > 
> > Did you remove this because NetUtil.newChannel does it automatically? Have
> > you tested debugging an add-on and see if the funky URLs appear or not?
> I removed this by accident, I'll add it back and write a test for it. I
> wonder why existing debugger tests didn't catch this one...

That's because such URLs only appear in chrome/add-on code and testing such code was impossible until very recently.
Comment on attachment 8621950 [details] [diff] [review]
bug-1170864-refactor-fetch.patch

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

::: toolkit/devtools/DevToolsUtils.js
@@ +485,4 @@
>    let deferred = promise.defer();
> +  let onResponse = (stream, status, request) => {
> +    if (!components.isSuccessCode(status)) {
> +      deferred.reject(new Error(`Failed to fetch ${aURL}. Code ${status}.`));

The failed attempt was for |url|, not |aURL|.
Attachment #8621950 - Flags: review?(past) → review+
Comment on attachment 8621953 [details] [diff] [review]
bug-1170864-fetch-tests.patch

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

Nice and thorough!
Attachment #8621953 - Flags: review?(past) → review+
Attached patch bug-1170864-refactor-fetch.patch (obsolete) — Splinter Review
Thanks for the reviews. Here's a new version that addresses your comment.
Attachment #8621950 - Attachment is obsolete: true
Attachment #8623647 - Flags: review+
Keywords: checkin-needed
The refactor patch needs rebasing.
Keywords: checkin-needed
Attached patch bug-1170864-refactor-fetch.patch (obsolete) — Splinter Review
Rebased. Even though the conflicting changeset contained mainly whitespace only changes to DevToolsUtils.js I had to make a few functional changes so that's why I'm asking you to take another look.

The conflicting revision, 002f738e36f0, changed following things:
1. It moved the fetch method code to an |if| block. I took the fetch method implementation in my patch, put it in a top-level function called mainThreadFetch and only added |exports.fetch = mainThreadFetch| inside that if block.
2. It changed one |catch (e) { if (e something) }| to |catch (e if e something) {}|. I changed it back the way it was originally.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c20fb98f216c
Attachment #8623647 - Attachment is obsolete: true
Attachment #8623760 - Flags: review?(past)
Comment on attachment 8623760 [details] [diff] [review]
bug-1170864-refactor-fetch.patch

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

Looks good, thanks.
Attachment #8623760 - Flags: review?(past) → review+
Keywords: checkin-needed
Great. The rebase went bad. Or rather, the rebased patch is missing a few changes from the original one. I'll have to look at it later.
Keywords: checkin-needed
All right, here's a patch that contains all pieces from the originally reviewed patch. The try run is quite colorful as I managed to pull broken fx-team and rebase the patches on top of that but all of the failures seem to be fixed already.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1fe19bb42e2
Attachment #8623760 - Attachment is obsolete: true
Attachment #8624590 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce27b34aeeba
https://hg.mozilla.org/mozilla-central/rev/a6d0ac7916ff
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1181345
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.