Need to figure out how cycle collected DOM files are supposed to play with workers

RESOLVED FIXED in Firefox 15

Status

()

Core
DOM
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: khuey, Assigned: janv)

Tracking

({regression})

unspecified
mozilla17
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 unaffected, firefox15+ verified, firefox16+ verified, firefox17+ verified, firefox-esr10 unaffected)

Details

Attachments

(1 attachment)

6.36 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
janv's file handle stuff created a cycle collected DOM file class, and baku has added another use of it.  This is great and all on the main thread, but trying to postMessage these to a worker thread causes Gecko to abort trying to AddRef a cycle collected object off the main thread.  We need to figure out a story here, ideally before janv's FileHandle ships in 15.
status-firefox14: --- → unaffected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
Blocks: 772434, 726593
Severity: normal → critical
(Assignee)

Comment 1

5 years ago
DOM files created by file handle are valid only on the main thread.
GetInternalStream() must be called to create a new internal request.
It means that these objects have to be passed to other APIs immediately in the success handler. It doesn't make sense to pass these objects to workers.
We can just throw in that case.
(Assignee)

Comment 2

5 years ago
I mean that GetInternalStream() must be called *synchronously* in the success handler.
Otherwise, if it is called later asynchronously, the method returns an error.

I think there's other possibility to handle passing of these objects to workers.
We could create a new thread safe DOM file object during serialization, that would have correct type, name, size, but GetInternalStream() would just throw.
(Assignee)

Comment 3

5 years ago
I'm not sure if blobs returned by the ArchiveReader() (should) work in DOM workers.
If they do, then we could actually create a thread safe blob wrapper that would forward method impl to the real cycle collected blob. Destructor of this wrapper would proxy release the cycle collected blob when needed.

Does that sound ok ?

Comment 4

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> We need to figure
> out a story here, ideally before janv's FileHandle ships in 15.

If left unfixed, what would the user impact be? Would backing out bug 726593 be a good alternative if this was left unfixed?
(Assignee)

Comment 5

5 years ago
I'm testing a fix, will attach a patch soon.

https://tbpl.mozilla.org/?tree=Try&rev=bbf5ec05ee07
(Assignee)

Comment 6

5 years ago
Created attachment 647226 [details] [diff] [review]
patch

fix per comment 1
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #647226 - Flags: review?(bent.mozilla)
(In reply to Jan Varga [:janv] from comment #3)
> I'm not sure if blobs returned by the ArchiveReader() (should) work in DOM
> workers.

They definitely should. A blob should always be a blob, there should be no differences between them.
I don't think a backout is a good idea at this point.  Those patches were pretty big.

Comment 9

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> I don't think a backout is a good idea at this point.  Those patches were
> pretty big.

Thanks for the clarification. What would the user impact be here?
The impact is that using some of the features introduced with web workers will cause Firefox to abort (crash in a non-exploitable fashion).
tracking-firefox15: ? → +
tracking-firefox16: ? → +
tracking-firefox17: ? → +
Comment on attachment 647226 [details] [diff] [review]
patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +371,5 @@
>        // Get the raw nsISupports out of it.
>        nsISupports* wrappedObject = wrappedNative->Native();
>        NS_ASSERTION(wrappedObject, "Null pointer?!");
>  
> +      nsISupports* isupports;

Nit: "ccISupports"? And let's initialize it to nullptr so we don't have to check for QI failure.

@@ +374,5 @@
>  
> +      nsISupports* isupports;
> +      wrappedObject->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
> +                                    reinterpret_cast<void**>(&isupports));
> +      if (!isupports) {

How about we do a warning here too:

  if (ccISupports) {
    NS_WARNING(...)
  }
  else {
    ...
Attachment #647226 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 12

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b83188166929
https://hg.mozilla.org/mozilla-central/rev/b83188166929
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
status-firefox17: affected → fixed
Please nominate for uplift so we can assess whether this will be able to land to branches, more specifically tomorrow's Beta.
(Assignee)

Comment 15

5 years ago
per discussion on IRC ...
The long term plan is to create a dummy threadsafe blob for blobs returned by FileHandle.getFile(), which would carry the name, type, size, etc.,
but it wouldn't be possible to read the blob in a worker, since the blob auto closed

However, on the other side, we should be able to pass ArchiveReader blobs to workers (and read content)
(Assignee)

Comment 16

5 years ago
Comment on attachment 647226 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 772434 and bug 726593

User impact if declined: 
Using some of the features introduced with web workers will cause Firefox to abort (crash in a non-exploitable fashion)

Testing completed (on m-c, etc.): 
Patch contains a mochitest and already landed on m-c

Risk to taking this patch (and alternatives if risky): 
There's very little risk. The patch logic is very simple and it just prevents cycle collected blobs (FileHandle and ArchiveReader ones) from passing to workers

String or UUID changes made by this patch: 
No string changes
Attachment #647226 - Flags: approval-mozilla-beta?
Attachment #647226 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

5 years ago
(In reply to Jan Varga [:janv] from comment #16)
> Risk to taking this patch (and alternatives if risky): 
> There's very little risk. The patch logic is very simple and it just
> prevents cycle collected blobs (FileHandle and ArchiveReader ones) from
> passing to workers

actually, only FileHandle ones, ArchiveReader is currently only on trunk
Comment on attachment 647226 [details] [diff] [review]
patch

low risk, has a test, approving.
Attachment #647226 - Flags: approval-mozilla-beta?
Attachment #647226 - Flags: approval-mozilla-beta+
Attachment #647226 - Flags: approval-mozilla-aurora?
Attachment #647226 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/940158b98658
https://hg.mozilla.org/releases/mozilla-beta/rev/edaab0f48feb
status-firefox-esr10: --- → unaffected
status-firefox15: affected → fixed
status-firefox16: affected → fixed
The mochitest attached to the fix passes on all systems:
* WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=14335951&tree=Mozilla-Beta
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=14336125&tree=Mozilla-Beta
* Mac OSX 10.7.4: https://tbpl.mozilla.org/php/getParsedLog.php?id=14334098&tree=Mozilla-Beta
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=14334298&tree=Mozilla-Beta

Marking verified for Firefox 15.0(beta).
status-firefox15: fixed → verified
Mihaela, can you please make sure this is verified for Firefox 16 and 17 as well? Thanks.
Keywords: verifyme
No fails for the attached test on Mozilla-Release(16.0) and Mozilla-Beta(17.0), either.

Mozilla-Release branch:
* WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=15999177&tree=Mozilla-Release
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=15998598&tree=Mozilla-Release
* Mac OSX 10.7: https://tbpl.mozilla.org/php/getParsedLog.php?id=15994856&tree=Mozilla-Release
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=15994814&tree=Mozilla-Release

Mozilla-Beta branch:
* Win XP: https://tbpl.mozilla.org/php/getParsedLog.php?id=16182170&tree=Mozilla-Beta
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=16181909&tree=Mozilla-Beta
* Mac OSX 10.7: https://tbpl.mozilla.org/php/getParsedLog.php?id=16176572&tree=Mozilla-Beta
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=16188220&tree=Mozilla-Beta
status-firefox16: fixed → verified
status-firefox17: fixed → verified
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.