Bug 93485

Summary: [Microdata] PropertyNodeList cache should be invalidated on id attribute change
Product: WebKit Reporter: Arko Saha <arko>
Component: DOMAssignee: Arko Saha <arko>
Status: RESOLVED FIXED    
Severity: Normal CC: naginenis, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 92986    
Attachments:
Description Flags
Patch rniwa: review+, webkit.review.bot: commit-queue-

Description Arko Saha 2012-08-08 07:56:48 PDT
PropertyNodeList cache is not getting invalidated when id attribute of an element changed/modified.

Test:
var parEl = makeEl('div',{},'<div itemprop="foo"><div itemprop="bar"></div></div><div itemscope itemref="id1"></div>');
var testEl = parEl.childNodes[1];
assert_equals( testEl.properties.namedItem('foo').length, 0, 'foo length (before test)' );
parEl.firstChild.id = 'id1';
assert_equals( testEl.properties.namedItem('foo').length, 1, 'foo length after id is created' );

Expected: Pass
Actual: Fail- assert_equals: foo length after id is created expected 1 but got 0
Comment 1 Arko Saha 2012-08-08 08:17:46 PDT
Created attachment 157222 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-09 00:29:21 PDT
Comment on attachment 157222 [details]
Patch

Rejecting attachment 157222 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Add optional debug logging for tiled scrolling

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13455807
Comment 3 Arko Saha 2012-08-09 00:48:21 PDT
Committed r125159: <http://trac.webkit.org/changeset/125159>
Comment 4 Sudarsana Nagineni (babu) 2012-08-10 06:24:29 PDT
I suspect this is caused an assertion on EFL debug bot. Could you check it, please? 

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4076

Stack Trace: 
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r125160%20%284076%29/fast/flexbox/001-crash-log.txt

These tests are flaky on the bot with the same stack trace.

fast/files/apply-blob-url-to-img.html = CRASH
fast/eventsource/eventsource-attribute-listeners.html = CRASH
fast/flexbox/001.html = CRASH
fast/flexbox/002.html = CRASH
Comment 5 Arko Saha 2012-08-10 09:06:42 PDT
(In reply to comment #4)
> I suspect this is caused an assertion on EFL debug bot. Could you check it, please? 
> 
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4076
> 
> Stack Trace: 
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r125160%20%284076%29/fast/flexbox/001-crash-log.txt
> 
> These tests are flaky on the bot with the same stack trace.
> 
> fast/files/apply-blob-url-to-img.html = CRASH
> fast/eventsource/eventsource-attribute-listeners.html = CRASH
> fast/flexbox/001.html = CRASH
> fast/flexbox/002.html = CRASH

Thanks for reporting the issue. I will analyze the root cause of the crash and update my analysis as soon as possible.
Comment 6 Ryosuke Niwa 2012-08-10 10:41:45 PDT
Comment on attachment 157222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157222&action=review

> Source/WebCore/dom/DynamicNodeList.h:71
> +    ALWAYS_INLINE bool isRootedAtDocument() const { return m_rootType == NodeListIsRootedAtDocument || ownerNodeHasItemRefAttribute(); }

Oops, this needs to just check if m_rootType is NodeListIsRootedAtDocumentIfOwnerHasItemrefAttr, not that it's currently rooted at the document.
Comment 7 Arko Saha 2012-08-10 11:02:48 PDT
(In reply to comment #6)
> (From update of attachment 157222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157222&action=review
> 
> > Source/WebCore/dom/DynamicNodeList.h:71
> > +    ALWAYS_INLINE bool isRootedAtDocument() const { return m_rootType == NodeListIsRootedAtDocument || ownerNodeHasItemRefAttribute(); }
> 
> Oops, this needs to just check if m_rootType is NodeListIsRootedAtDocumentIfOwnerHasItemrefAttr, not that it's currently rooted at the document.

Yes you are right. In Document::registerNodeListCache() it was not adding PropertyNodeList cache to m_listsInvalidatedAtDocument as it's currently not rooted at the document. Where in Document::unregisterNodeListCache() it was trying to remove PropertyNodeList cache from m_listsInvalidatedAtDocument. Hence it caused the assertion failure.
We should modify the check then.
Comment 8 Ryosuke Niwa 2012-08-10 11:12:55 PDT
Could you file a new bug and post a patch there?
Comment 9 Arko Saha 2012-08-10 11:32:24 PDT
(In reply to comment #8)
> Could you file a new bug and post a patch there?

Sure, filed bug : https://bugs.webkit.org/show_bug.cgi?id=93729