Incremental JavaScript source characters loading/decoding

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: yury, Assigned: yury)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(4 attachments, 18 obsolete attachments)

37.01 KB, patch
Details | Diff | Splinter Review
15.92 KB, patch
Details | Diff | Splinter Review
22.22 KB, patch
Details | Diff | Splinter Review
22.14 KB, patch
Details | Diff | Splinter Review
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)
Posted patch add-script-load-handler.patch (obsolete) — Splinter Review
Posted patch add-character-decoding.patch (obsolete) — Splinter Review
Depends on: 1217617
Posted patch add-script-load-handler.patch (obsolete) — Splinter Review
Allows getting (and not storing) the loaded data in chunks.
Attachment #8678388 - Attachment is obsolete: true
Posted patch add-character-decoding.patch (obsolete) — Splinter Review
(Now removes chars decoding from nsScriptLoader, however Subresource Integrity [SRI] still needs entire binary data to verify the integrity??)
Attachment #8678389 - Attachment is obsolete: true
Attachment #8678387 - Attachment is obsolete: true
Attachment #8678895 - Attachment is obsolete: true
Francois, can you provide feedback on the refactored code? See also 4-add-character-decoding.patch for SRICheckDataVerifier usage.
Attachment #8680261 - Flags: feedback?(francois)
Attachment #8678898 - Attachment is obsolete: true
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 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?
Attachment #8680261 - Flags: feedback?(francois)
Attachment #8680256 - Attachment is obsolete: true
unit test is added
Attachment #8680257 - Attachment is obsolete: true
(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.
Attachment #8680261 - Attachment is obsolete: true
(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
Attachment #8680262 - Attachment is obsolete: true
Attachment #8687422 - Flags: review?(mozilla)
Attachment #8687423 - Flags: review?(kvijayan)
Attachment #8687419 - Flags: review?(kvijayan)
Attachment #8687420 - Flags: review?(kvijayan)
Comment on attachment 8687422 [details] [diff] [review]
3-add-sricheckdataverifier.patch

Chattet with Francois over IRC - he is going to review that patch.
Attachment #8687422 - Flags: review?(mozilla) → review?(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"
Attachment #8687422 - Flags: review?(francois) → review+
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.
Attachment #8687419 - Flags: review?(kvijayan) → review+
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.
Attachment #8687420 - Flags: review?(kvijayan) → review+
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.
Attachment #8687423 - Flags: review?(kvijayan) → review+
Depends on: 1227947
(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.
Attachment #8687419 - Attachment is obsolete: true
(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.
Attachment #8687420 - Attachment is obsolete: true
(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.
Attachment #8687422 - Attachment is obsolete: true
(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)
Attachment #8687423 - Attachment is obsolete: true
Attachment #8692070 - Flags: review?(kvijayan)
Attachment #8692070 - Flags: review?(kvijayan) → review+
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!
Flags: needinfo?(ydelendik)
Keywords: checkin-needed
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
Attachment #8693701 - Attachment is obsolete: true
Flags: needinfo?(ydelendik)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.