Closed Bug 1397331 Opened 7 years ago Closed 7 years ago

window.URL.createObjectURL() no longer works in Firefox 57.0a1

Categories

(Core :: DOM: Core & HTML, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: dw-dev, Assigned: baku)

References

Details

(Keywords: regression, regressionwindow-wanted)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170809080026

Steps to reproduce:

I tested using my Save Page WE add-on with a large web page:
  https://www.reddit.com/r/IAmA/comments/66ajul/i_am_bill_nye_and_im_here_to_dare_i_say_it_save/

I tested with Chrome and with Firefox 55, 56 & latest 57.





Actual results:

Chrome: Save Page WE saved the page correctly.

Firefox 55 & 56: Save Page WE saved the page correctly.

Firefox 57:

Firefox hangs when Save Page WE is trying to create a Blob URL:

    htmlBlob = new Blob( htmlStrings, { type: "text/html" });
    objectURL = window.URL.createObjectURL(htmlBlob);

As far as I can see, no exception was thrown.

Up until a few days ago, the page was saved correctly and Firefox 57 did not hang.



Expected results:

Firefox 57:  Save Page WE should have saved the page correctly and Firefox should not have hung.
Please use mozregression to find the regression range: https://mozilla.github.io/mozregression/
Component: Untriaged → DOM
Product: Firefox → Core
Flags: needinfo?(amarchesini)
I'm not familiar with the regression tool and don't really have the time to get up to speed.

However, I have done a manual regression:

Save Page WE works with: Nightly 2017-08-25 10-01-26

Save Page WE fails with: Nightly 2017-08-26 10-04-18
Status: UNCONFIRMED → NEW
Ever confirmed: true
[Tracking Requested - why for this release]:
This bug really needs to be fixed in Firefox 57.

I have spent a considerable amount of time developing Save Page WE as a replacement for the Mozilla Archive Format and UnMHT add-ons, so that Firefox users still have the ability to save a complete web page as a single file.

Users who are migrating to Save Page WE have the expectation that it will work with Firefox 57, especially since it already works with Firefox 55 & 56.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch multiplex.patch (obsolete) — Splinter Review
Attachment #8905473 - Flags: review?(bugs)
What kind of test would have caught the regression?
Comment on attachment 8905473 [details] [diff] [review]
multiplex.patch

I need some explanation here what causes the bug and how this fixes it before I can review.
And a testcase would be really nice.
Attachment #8905473 - Flags: review?(bugs)
Comment on attachment 8905473 [details] [diff] [review]
multiplex.patch

The issue here is that we QI each sub-stream each time a new one is added or removed to/from a nsMultiplexInputStream.
Following the STR, firefox ends up creating a Blob with a nsMultiplexInputStream object containing more than 6000 substreams (probably because there are nested blobs). When that nsMultiplexInputStream is cloned, a new nsMultiplexInputStream is created and, for each sub-stream, a cloned version of them is appended. For each append, we do a QI of any other sub-stream. This is horrible.

The approach I'm suggesting is to have a simple data struct containing what a stream can do (is it cloneable? serializable? and so on). When the conditional QI is executed, this data struct is used.
Attachment #8905473 - Flags: review?(bugs)
Comment on attachment 8905473 [details] [diff] [review]
multiplex.patch

>   Mutex mLock; // Protects access to all data members.
>-  nsTArray<nsCOMPtr<nsIInputStream>> mStreams;
>+
>+  struct StreamData {
{ goes to its one line

>+    nsCOMPtr<nsIInputStream> mStream;
>+    bool mIsSeekable;
>+    bool mIsIPCSerializable;
>+    bool mIsCloneable;
>+    bool mIsAsyncInputStream;
>+
>+    void Popolate(nsIInputStream* aStream);
Popolate?
I guess you mean Populate

> NS_INTERFACE_MAP_BEGIN(nsMultiplexInputStream)
>   NS_INTERFACE_MAP_ENTRY(nsIMultiplexInputStream)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIInputStream, nsIMultiplexInputStream)
>-  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISeekableStream, mIsSeekable)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISeekableStream, IsSeekable())
>   NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream,
>-                                     mIsIPCSerializable)
>+                                     IsIPCSerializable())
>   NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsICloneableInputStream,
>-                                     mIsCloneable)
>+                                     IsCloneable())
>   NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIAsyncInputStream,
>-                                     mIsAsyncInputStream)
>+                                     IsAsyncInputStream())
Those Is* methods to iterate arrays, so can be a bit slow.
Could we rather just have counters for each optional interface type?
Attachment #8905473 - Flags: review?(bugs)
Attached patch multiplex2.patchSplinter Review
Attachment #8905473 - Attachment is obsolete: true
Attachment #8905514 - Flags: review?(bugs)
Comment on attachment 8905514 [details] [diff] [review]
multiplex2.patch

Please make NS_WARNING("ERROR: A nsIInputStream changed QI map when stored in a nsMultiplexInputStream!"); \
a MOZ_ASSERT


The macro is a bit odd. I'd let that to do the QIing based on the params passed to it.
I'd fix that.
Attachment #8905514 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8338a5c1ef21
Reduce the number of QI done in nsMultiplexInputStream using counters, r=smaug
https://hg.mozilla.org/mozilla-central/rev/8338a5c1ef21
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.