1218029 - Incremental JavaScript source characters loading/decoding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Yury Delendik (:yury)]

Subset of the bug 1154987: progressive loading and decoding JavaScript source characters. Currently we need to wait for entire source code to be loaded before we start decoding characters into utf-16 data (wide characters) then we parse entire code.

Expected to decode characters as soon as their arrive from a channel. (After that we can feed them in portions into JavaScript parser). It is expected that it will reduce strain on the memory when entire buffers for encoded and decoded data exist at the same time.

(Will take patches from the bug 1154987 as a base)

Comment 1

[Yury Delendik (:yury)]

Attachment: add-incremental-stream-loader.patch

Comment 2

[Yury Delendik (:yury)]

Attachment: add-script-load-handler.patch

Comment 3

[Yury Delendik (:yury)]

Attachment: add-character-decoding.patch

Comment 4

[Yury Delendik (:yury)]

Attachment: add-script-load-handler.patch

Allows getting (and not storing) the loaded data in chunks.

Comment 5

[Yury Delendik (:yury)]

Attachment: add-character-decoding.patch

(Now removes chars decoding from nsScriptLoader, however Subresource Integrity [SRI] still needs entire binary data to verify the integrity??)

Comment 6

[Yury Delendik (:yury)]

Attachment: 1-add-incremental-stream-loader.patch

Comment 7

[Yury Delendik (:yury)]

Attachment: 2-add-script-load-handler.patch

Comment 8

[Yury Delendik (:yury)]

Attachment: 3-add-sricheckdataverifier.patch

Francois, can you provide feedback on the refactored code? See also 4-add-character-decoding.patch for SRICheckDataVerifier usage.

Comment 9

[Yury Delendik (:yury)]

Attachment: 4-add-character-decoding.patch

Comment 10

[François Marier [:francois]]

Comment on attachment 8680262 [details] [diff] [review]
4-add-character-decoding.patch

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

::: dom/security/SRICheck.cpp
@@ -235,5 @@
> -
> -  if (MOZ_LOG_TEST(GetSriLog(), mozilla::LogLevel::Debug)) {
> -    nsAutoCString requestURL;
> -    request->GetName(requestURL);
> -    SRILOG(("SRICheck::VerifyIntegrity (stream), url=%s (length=%u)",

BTW, this has been a fairly useful log line while debugging SRI problems. If there's a way to preserve it, that would be great.

Comment 11

[François Marier [:francois]]

Comment on attachment 8680261 [details] [diff] [review]
3-add-sricheckdataverifier.patch

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

Overall, I'm fine with that refactoring and the question I would ask is: can we simplify the API a bit?

I mean, for incremental hash updates, everybody has to do:

1. Construct SRICheckDataVerifier()
2. Init()
3. Update()
4. Finish()
5. Verify()

Would it be worth simplifying this to?

1. Construct SRICheckDataVerifier() -- the work that Init() does would be rolled into this
2. Update()
3. Verify() -- this would start by doing what Finish() does

::: dom/security/SRICheck.cpp
@@ +365,5 @@
> +                             const nsIDocument* aDocument)
> +{
> +  NS_ENSURE_ARG_POINTER(aDocument);
> +
> +  if (NS_FAILED(IsEligible(aChannel, aCORSMode, aDocument))) {

Maybe this should be moved to Init() so that we can abort early?

Comment 12

[Yury Delendik (:yury)]

Attachment: 1-add-incremental-stream-loader.patch

Comment 13

[Yury Delendik (:yury)]

Attachment: 2-add-script-load-handler.patch

unit test is added

Comment 14

[Yury Delendik (:yury)]

Attachment: 3-add-sricheckdataverifier.patch

(In reply to François Marier [:francois] from comment #11)
 
> Would it be worth simplifying this to?
> 
> 1. Construct SRICheckDataVerifier() -- the work that Init() does would be
> rolled into this
> 2. Update()
> 3. Verify() -- this would start by doing what Finish() does

Fixed.

> > +  if (NS_FAILED(IsEligible(aChannel, aCORSMode, aDocument))) {
> Maybe this should be moved to Init() so that we can abort early?

Channel and CORS information might not be available during SRI metadata initialization stage.

Comment 15

[Yury Delendik (:yury)]

Attachment: 4-add-character-decoding.patch

(In reply to François Marier [:francois] from comment #10)
> > -    SRILOG(("SRICheck::VerifyIntegrity (stream), url=%s (length=%u)",
> BTW, this has been a fairly useful log line while debugging SRI problems. If
> there's a way to preserve it, that would be great.

This line added to SRICheckDataVerifier.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b690ee31b74

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8687422 [details] [diff] [review]
3-add-sricheckdataverifier.patch

Chattet with Francois over IRC - he is going to review that patch.

Comment 17

[François Marier [:francois]]

Comment on attachment 8687422 [details] [diff] [review]
3-add-sricheckdataverifier.patch

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

::: dom/security/SRICheck.cpp
@@ +301,5 @@
> +  nsresult rv;
> +  rv = EnsureCryptoHash();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mProcessed += aStringLen;

This count will be a lie if the Update() call fails. Not sure it matters though.

@@ +328,5 @@
> +SRICheckDataVerifier::VerifyHash(const SRIMetadata& aMetadata,
> +                                 uint32_t aHashIndex,
> +                                 const nsIDocument* aDocument)
> +{
> +  nsAutoCString base64Hash;

The original code had:

NS_ENSURE_ARG_POINTER(aDocument);

though I doubt we can get this far without a valid document...

::: dom/security/SRICheck.h
@@ +10,5 @@
>  #include "mozilla/CORSMode.h"
>  #include "nsCOMPtr.h"
> +#include "nsIChannel.h"
> +#include "nsICryptoHash.h"
> +#include "nsIDocument.h"

We should be able to use a forward declaration for nsIChannel and nsIDocument given they're both regular pointers.

@@ +69,5 @@
> +
> +  private:
> +    nsCOMPtr<nsICryptoHash> mCryptoHash;
> +    nsAutoCString           mComputedHash;
> +    size_t                  mProcessed;

nit: maybe a comment like "number of characters processed"

Comment 18

[Kannan Vijayan [:djvj]]

Comment on attachment 8687419 [details] [diff] [review]
1-add-incremental-stream-loader.patch

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

::: dom/base/nsScriptLoader.cpp
@@ +162,5 @@
>    // Unblock the kids, in case any of them moved to a different document
>    // subtree in the meantime and therefore aren't actually going away.
>    for (uint32_t j = 0; j < mPendingChildLoaders.Length(); ++j) {
>      mPendingChildLoaders[j]->RemoveExecuteBlocker();
>    }  

Nit: not your fault, but pre-existing whitespace at end of line.  Would be good to remove since you're touching the file anyway.

::: dom/xul/XULDocument.h
@@ +20,5 @@
>  #include "nsIURI.h"
>  #include "nsIXULDocument.h"
>  #include "nsScriptLoader.h"
>  #include "nsIStreamListener.h"
> +#include "nsIStreamLoader.h"

Is this include necessary?

::: netwerk/base/nsIncrementalStreamLoader.cpp
@@ +45,5 @@
> +NS_IMPL_ISUPPORTS(nsIncrementalStreamLoader, nsIIncrementalStreamLoader,
> +                  nsIRequestObserver, nsIStreamListener,
> +                  nsIThreadRetargetableStreamListener)
> +
> +NS_IMETHODIMP 

nit: whitespace at end of line.

@@ +53,5 @@
> +  return NS_OK;
> +}
> +
> +/* readonly attribute nsIRequest request; */
> +NS_IMETHODIMP 

nit: whitespace at end of line.

@@ +132,5 @@
> +}
> +
> +NS_IMETHODIMP 
> +nsIncrementalStreamLoader::OnDataAvailable(nsIRequest* request, nsISupports *ctxt, 
> +                                nsIInputStream *inStr, 

Nit: whitespace at end of line.

Comment 19

[Kannan Vijayan [:djvj]]

Comment on attachment 8687420 [details] [diff] [review]
2-add-script-load-handler.patch

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

::: dom/base/nsScriptLoader.h
@@ +342,5 @@
>                                   char16_t*& aBufOut, size_t& aLengthOut);
>  
>    /**
> +   * Handle the completion of a stream.  This is called by the
> +   * ContextMediator object which observes the IncrementalStreamLoader

You removed |ContextMediator|.  I think you mean "This is called by nsScriptLoadHandler ...".

::: netwerk/base/nsIIncrementalStreamLoader.idl
@@ +27,5 @@
> +     * the internal buffer, the consumedLength shall be set to the value of
> +     * the dataLength or less. By default the consumedLength value is assumed 0.
> +     * The data and dataLength reflect the non-consumed data and will be
> +     * accumulated if consumedLength is not set.
> +     * 

Nit: whitespace at end of line.

::: netwerk/test/unit/test_bug1218029.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> + 
> +var tests = [

Nit: remove all whitespace at end of line in file.

Comment 20

[Kannan Vijayan [:djvj]]

Comment on attachment 8687423 [details] [diff] [review]
4-add-character-decoding.patch

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

::: dom/base/nsScriptLoader.cpp
@@ +1876,4 @@
>  {
> +  DebugOnly<nsresult> rv = NS_OK;
> +  if (EnsureDecoder(aLoader, aData, aDataLength,
> +                    /* aEndOfStream = */ true)) {

In this last case, EnsureDecoder should always return true.  The result of calling |EnsureDecoder| here should be asserted true instead of conditionalized.

Comment 21

[Yury Delendik (:yury)]

Attachment: 1-add-incremental-stream-loader.patch

(In reply to Kannan Vijayan [:djvj] from comment #18)
> Comment on attachment 8687419 [details] [diff] [review]
> 1-add-incremental-stream-loader.patch
> 
> Review of attachment 8687419 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/xul/XULDocument.h
> @@ +20,5 @@
> >  #include "nsIURI.h"
> >  #include "nsIXULDocument.h"
> >  #include "nsScriptLoader.h"
> >  #include "nsIStreamListener.h"
> > +#include "nsIStreamLoader.h"
> 
> Is this include necessary?

Yes. We removed nsIStreamLoader.h include from the nsScriptLoader.h, but XULDocument.h uses nsIStreamLoader class below.

Comment 22

[Yury Delendik (:yury)]

Attachment: 2-add-script-load-handler.patch

(In reply to Kannan Vijayan [:djvj] from comment #19)
> Comment on attachment 8687420 [details] [diff] [review]
> 2-add-script-load-handler.patch
> 
> Review of attachment 8687420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsScriptLoader.h
> @@ +342,5 @@
> >                                   char16_t*& aBufOut, size_t& aLengthOut);
> >  
> >    /**
> > +   * Handle the completion of a stream.  This is called by the
> > +   * ContextMediator object which observes the IncrementalStreamLoader
> 
> You removed |ContextMediator|.  I think you mean "This is called by
> nsScriptLoadHandler ...".

Fixed (along with nits).


Trivial fix is added to correct the numBytesRead calculation.

Comment 23

[Yury Delendik (:yury)]

Attachment: 3-add-sricheckdataverifier.patch

(In reply to François Marier [:francois] from comment #17)
> Comment on attachment 8687422 [details] [diff] [review]
> 3-add-sricheckdataverifier.patch
> 
> Review of attachment 8687422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/SRICheck.cpp
> @@ +301,5 @@
> > +  nsresult rv;
> > +  rv = EnsureCryptoHash();
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  mProcessed += aStringLen;
> 
> This count will be a lie if the Update() call fails. Not sure it matters
> though.

I renamed mProcessed into mBytesHashed to better reflect what it means.

> 
> @@ +328,5 @@
> > +SRICheckDataVerifier::VerifyHash(const SRIMetadata& aMetadata,
> > +                                 uint32_t aHashIndex,
> > +                                 const nsIDocument* aDocument)
> > +{
> > +  nsAutoCString base64Hash;
> 
> The original code had:
> 
> NS_ENSURE_ARG_POINTER(aDocument);
> 
> though I doubt we can get this far without a valid document...

I added the assert back.

> 
> ::: dom/security/SRICheck.h
> @@ +10,5 @@
> >  #include "mozilla/CORSMode.h"
> >  #include "nsCOMPtr.h"
> > +#include "nsIChannel.h"
> > +#include "nsICryptoHash.h"
> > +#include "nsIDocument.h"
> 
> We should be able to use a forward declaration for nsIChannel and
> nsIDocument given they're both regular pointers.

Fixed.

> 
> @@ +69,5 @@
> > +
> > +  private:
> > +    nsCOMPtr<nsICryptoHash> mCryptoHash;
> > +    nsAutoCString           mComputedHash;
> > +    size_t                  mProcessed;
> 
> nit: maybe a comment like "number of characters processed"

The variable name changed to mBytesProcessed.

Comment 24

[Yury Delendik (:yury)]

Attachment: 4-add-character-decoding.patch

(In reply to Kannan Vijayan [:djvj] from comment #20)
> Comment on attachment 8687423 [details] [diff] [review]
> 4-add-character-decoding.patch
> 
> Review of attachment 8687423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsScriptLoader.cpp
> @@ +1876,4 @@
> >  {
> > +  DebugOnly<nsresult> rv = NS_OK;
> > +  if (EnsureDecoder(aLoader, aData, aDataLength,
> > +                    /* aEndOfStream = */ true)) {
> 
> In this last case, EnsureDecoder should always return true.  The result of
> calling |EnsureDecoder| here should be asserted true instead of
> conditionalized.

Fixed.

I found an issue with ReadSegments's callback returning error codes (see bug 1227947) -- fails one crashtest for debug builds. I had to add a workaround and just ignore the incoming data when request is cancelled.

(Newer try https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b5ae4749c0&selectedJob=14106754)

Comment 25

[Yury Delendik (:yury)]

Attachment: Part 1. Adds IncrementalStreamLoader interface stubs

Comment 26

[Yury Delendik (:yury)]

Attachment: Part 2. Adds ScriptLoadHandler and implements OnIncrementalData callback

Comment 27

[Yury Delendik (:yury)]

Attachment: Part 3. Adds SRICheckDataVerifier for progressing data handling

Comment 28

[Yury Delendik (:yury)]

Attachment: Part 4. Implements progressive Unicode chars decoding in nsScriptLoader

Comment 29

[Yury Delendik (:yury)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f59788af168c

Comment 30

[Carsten Book [:Tomcat]]

i think part 2 cause problems:

Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIIncrementalStreamLoaderObserver in changeset 4bc3ca761e17
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIIncrementalStreamLoaderObserver


could you take a look, thanks!

Comment 31

[Yury Delendik (:yury)]

Attachment: Part 2. Adds ScriptLoadHandler and implements OnIncrementalData callback

Part 1 creates incomplete implementation/stubs for the IncreamentalStreamLoader and part 2 modifies the implementation to add a desired method. I added UUID change to the part 2 to satisfy verifier's needs.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8f1cb10f01

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73cfba395b86
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb6aac48d4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7adc1f24f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb2a3f0fb90

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73cfba395b86
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb6aac48d4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7adc1f24f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb2a3f0fb90

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73cfba395b86
https://hg.mozilla.org/mozilla-central/rev/6fb6aac48d4d
https://hg.mozilla.org/mozilla-central/rev/7a7adc1f24f8
https://hg.mozilla.org/mozilla-central/rev/2bb2a3f0fb90