Closed
Bug 1389913
Opened 7 years ago
Closed 7 years ago
domstringlist.html wpt test fails with harness error in Firefox but not Chrome due to IndexedDB exception
Categories
(Core :: Storage: IndexedDB, defect, P3)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ayg, Assigned: baku)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
http://w3c-test.org/html/infrastructure/common-dom-interfaces/collections/domstringlist.html
It seems like the indexeddb.open() call is returning an AbortError in such a fashion that it's caught by the global error handler in the same way as an uncaught exception, even though an onerror handler is specified on a subsequent line. Chrome and Edge don't do this. What's happening?
This seems related to bug 1120178 comment #5.
Comment 2•7 years ago
|
||
Looks like the issue is that idbfactory errors are always reported on the window if the thing they were fired on does not preventDefault. See IndexedDatabaseManager::CommonPostHandleEvent and sgo->HandleScriptError() call in there.
We also fire error events on the global in workers, looks like.
I don't see anything in the idb spec saying we should do that, and it's not clear to me why we're doing it.
Comment 3•7 years ago
|
||
Andrea, do you know what the error reporting story here is and why we do it?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 4•7 years ago
|
||
> We also fire error events on the global in workers, looks like.
>
> I don't see anything in the idb spec saying we should do that, and it's not
> clear to me why we're doing it.
This seems old code. Wondering if the spec was saying that before indexedDB-2.
I take this bug.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 5•7 years ago
|
||
Flags: needinfo?(jvarga)
Attachment #8912144 -
Flags: review?(bugmail)
Comment 6•7 years ago
|
||
Comment on attachment 8912144 [details] [diff] [review]
idb.patch
:bevis, it looks like your work on bug 1274938 and the spec discussion on https://github.com/w3c/IndexedDB/issues/49 that you're involved with is still relevant, so I'm transferring this review to you.
One thing worth noting is that even if we want to remove the window.onerror firing at content, this also removes the ScriptErrorHelper::Dump() that I think in various cases has helped us diagnose weird IDB problems. We should make sure we've got IDB_WARNING paths for everything we care about if removing it.
Attachment #8912144 -
Flags: review?(bugmail) → review?(btseng)
Comment 7•7 years ago
|
||
Comment on attachment 8912144 [details] [diff] [review]
idb.patch
Review of attachment 8912144 [details] [diff] [review]:
-----------------------------------------------------------------
The implementation was done in bug 607729 and the only information I have is on bug 1274938 comment 6 before :sicking leaved. :\
This feature seems wanted for a long time by both Chrome/Firefox according to https://github.com/w3c/IndexedDB/issues/49#issue-110290805 but we didn't have it specified in v2(the milestone now is set to v3).
Instead of removing the implementation entirely, I'd prefer to have a preference to enable firing (window|self).onerror in
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/indexedDB/IndexedDatabaseManager.cpp#549-594
and the default value of this preference is false for now.
Then, we could still have error printed in the error console for developers to debug:
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/indexedDB/IndexedDatabaseManager.cpp#596-603
Does this make sense to you?
Attachment #8912144 -
Flags: review?(btseng)
Comment 8•7 years ago
|
||
Here is the naming rule of idb preference for your reference:
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/indexedDB/IndexedDatabaseManager.cpp#142-171
Assignee | ||
Comment 9•7 years ago
|
||
Do you mind to file a spec issue?
Attachment #8912144 -
Attachment is obsolete: true
Attachment #8912529 -
Flags: review?(btseng)
Comment 10•7 years ago
|
||
Comment on attachment 8912529 [details] [diff] [review]
idb2.patch
Review of attachment 8912529 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! BTW, the spec issue is already available in https://github.com/w3c/IndexedDB/issues/49
Attachment #8912529 -
Flags: review?(btseng) → review+
Comment 11•7 years ago
|
||
Ok, thanks for fixing this.
Comment 12•7 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7403204c8fa
IDB should not propagate the error events to self.onerror, r=bevis
Comment 13•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•