nsIJSON.encodeToStream() broken

RESOLVED FIXED in mozilla5

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Assigned: Emanuele Costa)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla5
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(4 attachments)

After trying to make nsIJSON.encodeToStream() work for some time I realized that it is not my fault - the function is broken, it doesn't work at all. From code inspection the regression appears to have been introduced in bug 410890, it's this change:

> -nsJSONWriter::nsJSONWriter(nsIOutputStream *aStream) : mStream(aStream),
> +nsJSONWriter::nsJSONWriter(nsIOutputStream *aStream) : mStream(nsnull),
> +                                                       mBuffer(nsnull),
> +                                                       mBufferCount(0),
> +                                                       mDidWrite(PR_FALSE),
>                                                         mEncoder(nsnull)

Note that this didn't only add more variables to initialize, it also made aStream parameter ignored - mStream is now being initialized with nsnull. I wondered why the issue hasn't been detected, after all /dom/src/json/test/unit/test_encode.js is testing this function. But looking closer the contents of the file written aren't being checked at all - only the file size (which is zero so we get all the expected proportions between encodings).
blocking2.0: --- → ?
Boris, thanks for requesting blocking2.0 flag. Reason why I didn't do it myself - the regression happened three years ago and apparently nobody noticed. Still, this should be a simple and safe change assuming that nothing other than the initialization broke in the three years. It would be nice to have this essential API finally working in Firefox 4.
Not a blocker, since it's not a regression vs 3.6.  Sorry; we can get it in FF5, if we get a good patch for then.
blocking2.0: ? → -

Updated

7 years ago
Assignee: nobody → sayrer
(Assignee)

Comment 3

7 years ago
Hi, few problems there. The bug is not detected anymore because the corresponding testing function is currently commented out:


   // failing on windows -- bug 410005
-  // testOutputStreams();
+  testOutputStreams();
   
 }

Running the test cause the following failure:

TEST-UNEXPECTED-FAIL | /home/emanuele/mozilla-central/objdir-ff-debug/_tests/xpcshell/dom/src/json/test/unit/test_encode.js | 3 == 5 - See following stack:
JS frame :: /home/emanuele/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 439
JS frame :: /home/emanuele/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 491
JS frame :: /home/emanuele/mozilla-central/objdir-ff-debug/_tests/xpcshell/dom/src/json/test/unit/test_encode.js :: testOutputStreams :: line 152
JS frame :: /home/emanuele/mozilla-central/objdir-ff-debug/_tests/xpcshell/dom/src/json/test/unit/test_encode.js :: run_test :: line 181
JS frame :: /home/emanuele/mozilla-central/testing/xpcshell/head.js :: _execute_test :: line 322
JS frame :: -e :: <TOP_LEVEL> :: line 1

I inspected the C++ code and added the correct initialisation parameter (nsnull is definitely wrong), the tests all pass now:


-nsJSONWriter::nsJSONWriter(nsIOutputStream *aStream) : mStream(nsnull),
+nsJSONWriter::nsJSONWriter(nsIOutputStream *aStream) : mStream(aStream),
                                                        mBuffer(nsnull),
                                                        mBufferCount(0),
                                                        mDidWrite(PR_FALSE),

Given that the test was disabled because of a windows related issue/bug I will now compile it on windows and check the behaviour there.

Also, as discussed, the xpcshell test could be extended to not only check file existence but size and maybe content so I will devote a bit of time to that as well to make the test more robust and check all the behaviours inside the class.
(Assignee)

Comment 4

7 years ago
ok the file sizes are actually checked in the test script, so all the tests passed and patch seem to be limited to the initialisation argument.
(Assignee)

Comment 5

7 years ago
Created attachment 522573 [details] [diff] [review]
patch for wrong initialisation parameter

Tested locally by running all regression tests (xpcshell) and now they all work (but second patch in test_encode.js is needed to run the tests). I am now applying for a mozilla account to use the "Try Server"
(Assignee)

Comment 6

7 years ago
Created attachment 522575 [details] [diff] [review]
patch to add back the outputstream tests

second patch necessary to run the outputstream tests (previously disabled)
Comment on attachment 522573 [details] [diff] [review]
patch for wrong initialisation parameter

r=me

For future reference, it's a good idea to make sure the From and the commit message are set on patches.  The best way to do that is to hg export them or upload them directly from an mq (or to use bzexport).
Attachment #522573 - Flags: review+
Comment on attachment 522575 [details] [diff] [review]
patch to add back the outputstream tests

Going to push this to try, but if it's passing there we should remove the no-longer-correct comment too.
OK, on try server the test fails even with the patch:

TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/dom/src/json/test/unit/test_encode.js | 0 == 5 - See following stack:
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_throw :: line 439
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_check_eq :: line 491
JS frame :: c:/talos-slave/test/build/xpcshell/tests/dom/src/json/test/unit/test_encode.js :: testOutputStreams :: line 142
JS frame :: c:/talos-slave/test/build/xpcshell/tests/dom/src/json/test/unit/test_encode.js :: run_test :: line 167
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: _execute_test :: line 322
(Assignee)

Comment 10

7 years ago
Ok, so a windows issue now. Will build and try on windows.
(Assignee)

Comment 11

7 years ago
The patch works both on Linux and windows. The issue on windows is different and not a json issue. It is a xpcom/io fileSize issue.

If a file is created using the function writeToFile in test_encode.js it will always return a fileSize = 0 but the file is actually created correctly and with the correct content. I wrote a small function to load all the files and decode them back and I can confirm it. 

I also added few printf statements to nsLocalFileWin.cpp and indeed proved that 
GetFileSize mFileInfo64 will be incorrectly set to 0 after the file is being created by:

 function writeToFile(obj, charset, writeBOM) {
    var jsonFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
    jsonFile.initWithFile(outputDir);
    jsonFile.append("test.json");
    jsonFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0600);
    var stream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
    try {
      stream.init(jsonFile, 0x04 | 0x08 | 0x20, 0600, 0); // write, create, truncate
      nativeJSON.encodeToStream(stream, charset, writeBOM, obj);
    } finally {
      stream.close();
    }
    print('file size orig: ' + jsonFile.fileSize);
    return jsonFile;
  }

Also notice the print I put above that again confirms the bug. 

So a different bug I will now investigate but the json patch can be added.
nsIFile caches file data, maybe an old fileSize is cached? How about checking jsonFile.clone().fileSize, will it work correctly?
(Assignee)

Comment 13

7 years ago
Ok, I think I found it. The problem seems to be in the nsLocalFileWin implementation, more specifically the mDirty flag.

With the mDirty logic on, the following happens:

here3
ResolveAndStat entered!
ResolveAndStat mDirty!
GetFileInfo name C:\DOCUME~1\Emanuele\LOCALS~1\Temp\json-test-output\test-3262.json
GetFileInfo nFileSizeHigh 0
GetFileInfo nFileSizeHigh 0
GetFileInfo info->size 0
ResolveAndStat GetFileInfo!
EncodeToStream ignored 3
here4
GetFileSize entered!
ResolveAndStat entered!
GetFileSize mFileInfo64 0
file size 0

meaning that GetFileInfo is called before the actual data is writing (just after file creation) and later on the mDirty flag will prevent the GetFileInfo to be called again.

Commenting out the mDirty check everything works:

here3
ResolveAndStat entered!
ResolveAndStat mDirty!
GetFileInfo name C:\DOCUME~1\Emanuele\LOCALS~1\Temp\json-test-output\test-3264.j
son
GetFileInfo nFileSizeHigh 0
GetFileInfo nFileSizeHigh 0
GetFileInfo info->size 0
ResolveAndStat GetFileInfo!
EncodeToStream ignored 3
here4
GetFileSize entered!
ResolveAndStat entered!
ResolveAndStat mDirty!
GetFileInfo name C:\DOCUME~1\Emanuele\LOCALS~1\Temp\json-test-output\test-3264.j
son
GetFileInfo nFileSizeHigh 0
GetFileInfo nFileSizeHigh 5
GetFileInfo info->size 5
ResolveAndStat GetFileInfo!
GetFileSize mFileInfo64 5
file size 5

GetFileInfo is called twice and the second time the filesize set properly and all the tests pass also on windows.

The Linux implementation does not have this problem (no mDirty logic).

Notice the mDirty bug is caused in the test by calling ResolveAndStat soon after file creation:

 var jsonFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
    jsonFile.initWithFile(outputDir);
    jsonFile.append("test.json");
    jsonFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0600);
    var stream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
 nativeJSON.encodeToStream(stream, charset, writeBOM, obj);

the first call before encodeToStream set mDirty to  mDirty = PR_FALSE;

This is a funny issue in my opinion and should have been detected earlier so there may be something in the design of the mDirty logic I am not aware of, but the problem is this early call of GetFileInfo and mDirty.

Any ideas?
That's what I meant - if you changed the file and need current info you should call clone().
(Assignee)

Comment 15

7 years ago
yes I am looking at mDirty and MakeDirty and the test_encode.js and it happens
because the writeBOM parameter set to true (in that case extra info are added
to the file) like in:

var f = writeToFile({},"UTF-16BE", true);

while other calls as:

 var utf16LEFile = writeToFile(pair[1], "UTF-16LE", false);

do not cause the bug.

So, I could fix the test by adding clone but this will make the whole class
fragile. 

Wouldn't it better to solve this at class level?

I will try to provide two patches.
(Assignee)

Comment 16

7 years ago
Created attachment 523900 [details] [diff] [review]
patch working for windows as well

overall patch (first hg export patch)
Comment on attachment 523900 [details] [diff] [review]
patch working for windows as well

This looks fine if you remove the comment about failing and instead add a comment after "check BOMs" saying that the clone() calls are there to work around bug 410005.  Also the commit message should describe the overall fix (including the bug number), not just the latest change.

And you should probably request review by setting the "review" flag on the patch to '?' and filling in the requestee's e-mail instead of sending private mail.

I'll push this to try server.
Attachment #523900 - Flags: review+
Pushed try changeset 352e43ec2c30.  I'll see what things look like when I get off the plane tonight....
That last patch passes Windows xpcshell tests on Try.

Emanuele, can you update the comments and commit message please?
(Assignee)

Comment 20

7 years ago
Created attachment 523941 [details] [diff] [review]
patch for 633934 bug with comments for related 410005
Attachment #523941 - Flags: review?(bzbarsky)
Comment on attachment 523941 [details] [diff] [review]
patch for 633934 bug with comments for related 410005

This looks great.  Thanks!
Attachment #523941 - Flags: review?(bzbarsky) → review+
Pushed this to cedar; should get merged into mozilla-central in the next day or two: http://hg.mozilla.org/projects/cedar/rev/5e74b6d598e7

Emanuele, thanks again for the patch!
Assignee: sayrer → emanuele.costa
Flags: in-testsuite+
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/5e74b6d598e7
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.